Skip to content
This repository has been archived by the owner on Jan 25, 2023. It is now read-only.

Support Urho3D version like SDL #2525

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Conversation

Y-way
Copy link
Contributor

@Y-way Y-way commented Oct 18, 2019

No description provided.

@iSLC
Copy link
Contributor

iSLC commented Oct 18, 2019

While this is neat. I don't see people dealing with backwards compatibility. Or the engine itself.

The way I see it is you pick a version and stick with it. Or maybe upgrade in the future. But not really go back.

Going back is like asking to bring back fixed issues.

SDL needs this because there are wrappers and other such things that need to deal with compatibility across multiple versions of the library.

I could be wrong. So feel free to ignore my thoughts.

@weitjong
Copy link
Contributor

Although I like the idea, there are a number of issues in the implementation as apparently the change has broken the CI build test. For one, as you may have already known, the "minor" part is now not necessarily a number anymore (consider 1.8-ALPHA for example). You have commented out a logic to prevent the generated header from changing its timestamp and therefore the new change might trigger a rebuild unnecessarily (if the content is stale then user can easily do a "make clean" to mitigate rather than removing this logic).

I don't think I will merge this PR as it is but because of it at least it pointed out we have a bug in version parsing to cater for -ALPHA/BETA/RC. So thanks for that.

@Y-way
Copy link
Contributor Author

Y-way commented Oct 19, 2019

@weitjong Thanks for your reply.

I don't think I will merge this PR as it is but because of it at least it pointed out we have a bug in > > version parsing to cater for -ALPHA/BETA/RC. So thanks for that.

I'm not sure what you mean.
I move the below codes
https://github.com/urho3d/Urho3D/blob/3ad625a459156bafd9b16b43df9aa6cc02cb0233/CMake/Modules/GetUrhoRevision.cmake#L43-L57
to
https://github.com/urho3d/Urho3D/blob/3ad625a459156bafd9b16b43df9aa6cc02cb0233/CMake/Modules/GetUrhoRevision.cmake#L37-L38

and I didn`t change the version parsing.

@weitjong
Copy link
Contributor

weitjong commented Oct 19, 2019

I can see what you tried to do and understand your intend, moving the existing code up before the if() branch. However, what I wanted to say is, that existing code was actually out-dated. My last commits bring the parsing logic up to speed now. But I see the latest commit in your PR just nullifying my work.

@Y-way
Copy link
Contributor Author

Y-way commented Oct 19, 2019

@weitjong Oh, sorry. I see.
I will check it again.

@github-actions
Copy link

github-actions bot commented Mar 7, 2020

Stale pull request message

@weitjong weitjong added backlog улучшение Улучшение существующих вещей and removed no-pr-activity labels Mar 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backlog улучшение Улучшение существующих вещей
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants