New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix FindSFML.cmake can't find SFML 2.1 #903

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@xelez
Contributor

xelez commented Jun 2, 2015

After commit 1c46ec7 that updates version to 2.2, FindSFML.cmake can't find SFML 2.1 (and maybe other versions too).

That happens because Config.hpp of 2.1 hasn't got define for SFML_VERSION_PATCH. So regexp in FindSFML.cmake finds nothing.

I deleted first REGEX MATCH, because it seems to be just a little optimization, that gets things more complicated.

@mantognini mantognini added this to the 2.3.2 milestone Jul 4, 2015

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Jul 4, 2015

Member

Works fine for me. EDIT: see comments on code

I've added it to 2.3.2 task list.

Member

mantognini commented Jul 4, 2015

Works fine for me. EDIT: see comments on code

I've added it to 2.3.2 task list.

Show outdated Hide outdated cmake/Modules/FindSFML.cmake Outdated

@mantognini mantognini removed the s:accepted label Jul 4, 2015

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Jul 31, 2015

Member

Bump.

Member

binary1248 commented Jul 31, 2015

Bump.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Aug 4, 2015

Member

@xelez Can you update the PR? Otherwise we'll have to create a new PR ourselves. 😉

Member

eXpl0it3r commented Aug 4, 2015

@xelez Can you update the PR? Otherwise we'll have to create a new PR ourselves. 😉

@xelez

This comment has been minimized.

Show comment
Hide comment
@xelez

xelez Aug 4, 2015

Contributor

Fixed, checked it on 2.0, 2.1, 2.2, 2.3, 2.3.1, hope I've missed nothing this time.

P.S. Oh, em.. sorry for long waiting, I'm quite ashamed both for the response time and that stupid mistake. :(

Contributor

xelez commented Aug 4, 2015

Fixed, checked it on 2.0, 2.1, 2.2, 2.3, 2.3.1, hope I've missed nothing this time.

P.S. Oh, em.. sorry for long waiting, I'm quite ashamed both for the response time and that stupid mistake. :(

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Aug 4, 2015

Member

Thanks! No need to feel bad.

Could you squash the two commits into one?

Member

eXpl0it3r commented Aug 4, 2015

Thanks! No need to feel bad.

Could you squash the two commits into one?

@xelez

This comment has been minimized.

Show comment
Hide comment
@xelez

xelez Aug 4, 2015

Contributor

@eXpl0it3r like that?

Contributor

xelez commented Aug 4, 2015

@eXpl0it3r like that?

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Aug 4, 2015

Member

Yes!

This PR has been added to my merge list, meaning it will be merged soon into the branch 2.3.x, unless someone raises any concerns.

Member

eXpl0it3r commented Aug 4, 2015

Yes!

This PR has been added to my merge list, meaning it will be merged soon into the branch 2.3.x, unless someone raises any concerns.

@eXpl0it3r eXpl0it3r self-assigned this Aug 5, 2015

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Aug 5, 2015

Member

Now that I test it, I wonder what exactly we "gain" from this, because if you try to use for example SFML 2.1 with the FindSFML.cmake from this PR, it may find SFML, but it will still search for OGG, Vorbis, FLAC, etc. even though they are not used with SFML 2.1...

Should I apply it anyways?

Member

eXpl0it3r commented Aug 5, 2015

Now that I test it, I wonder what exactly we "gain" from this, because if you try to use for example SFML 2.1 with the FindSFML.cmake from this PR, it may find SFML, but it will still search for OGG, Vorbis, FLAC, etc. even though they are not used with SFML 2.1...

Should I apply it anyways?

@xelez

This comment has been minimized.

Show comment
Hide comment
@xelez

xelez Aug 5, 2015

Contributor

Well, if someone uses SFML and cmake in their project, I think what they usually do is just copy this file to their own CMake Modules directory (I don't use SFML by myself, maybe some day =)). And now when someone wants to build that project against default libraries on, for example, Ubuntu they see quite strange error about can't find SFML and think "Eeerm.. what? Here it is! Why can't it find it?". That's how I found this bug. And I thought that after I fixed it in my case I should share the solution.

On the other hand... if someone uses SFML 2.2 and uses current FindSFML... maybe it's not quite clever to build against older version than the developer had used, even if it's said it's supported.

And in about a year or probably less most of the people will use and support only SFML 2.2 or later, so if there is "gain", then it's probably very small.

Oh, and another thought. If current version of FindSFML fails on 2.1 or less.. maybe a better approach is to cleanup this file and make it clear, that it supports only 2.2 or later?

By the way, here http://www.sfml-dev.org/tutorials/2.3/start-linux.php it's suggested to apt-get install libsfml-dev... and it's 2.1 in ubuntu 14.04 for example.

Contributor

xelez commented Aug 5, 2015

Well, if someone uses SFML and cmake in their project, I think what they usually do is just copy this file to their own CMake Modules directory (I don't use SFML by myself, maybe some day =)). And now when someone wants to build that project against default libraries on, for example, Ubuntu they see quite strange error about can't find SFML and think "Eeerm.. what? Here it is! Why can't it find it?". That's how I found this bug. And I thought that after I fixed it in my case I should share the solution.

On the other hand... if someone uses SFML 2.2 and uses current FindSFML... maybe it's not quite clever to build against older version than the developer had used, even if it's said it's supported.

And in about a year or probably less most of the people will use and support only SFML 2.2 or later, so if there is "gain", then it's probably very small.

Oh, and another thought. If current version of FindSFML fails on 2.1 or less.. maybe a better approach is to cleanup this file and make it clear, that it supports only 2.2 or later?

By the way, here http://www.sfml-dev.org/tutorials/2.3/start-linux.php it's suggested to apt-get install libsfml-dev... and it's 2.1 in ubuntu 14.04 for example.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Aug 12, 2015

Member

If nobody has anything against it, I'll merge this, even if it only fixes things partially - FindSFML.cmake will still search for the wrong dependencies when choosing SFML 2.0/2.1/2.2.

Member

eXpl0it3r commented Aug 12, 2015

If nobody has anything against it, I'll merge this, even if it only fixes things partially - FindSFML.cmake will still search for the wrong dependencies when choosing SFML 2.0/2.1/2.2.

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Aug 12, 2015

Member

It won't hurt, but a new issue has to be created that establishes backwards compatibility.

Member

TankOs commented Aug 12, 2015

It won't hurt, but a new issue has to be created that establishes backwards compatibility.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Aug 13, 2015

Member

Looking at the code again, it still seems to be wrong. Before we tried to match the major, minor and patch version in the config file. Now it just replaces things and afterwards checks if the patch version is set or not. Meaning we do not actually check first if the #defines even exists.

Member

eXpl0it3r commented Aug 13, 2015

Looking at the code again, it still seems to be wrong. Before we tried to match the major, minor and patch version in the config file. Now it just replaces things and afterwards checks if the patch version is set or not. Meaning we do not actually check first if the #defines even exists.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Aug 17, 2015

Member

Can anyone else confirm that this code is good (or bad)?

Member

eXpl0it3r commented Aug 17, 2015

Can anyone else confirm that this code is good (or bad)?

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Aug 17, 2015

Member

If it doesn't help in the end, then it's of no big use. It can be applied, but like said, to properly support older versions, we will have to add branching to FindSFML.cmake to cover different dependencies etc.

Member

TankOs commented Aug 17, 2015

If it doesn't help in the end, then it's of no big use. It can be applied, but like said, to properly support older versions, we will have to add branching to FindSFML.cmake to cover different dependencies etc.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Aug 17, 2015

Member

No I mean does the current code actually work. I mean the first statement has been completely removed and note replaced with anything else. So I'm just wondering.

Member

eXpl0it3r commented Aug 17, 2015

No I mean does the current code actually work. I mean the first statement has been completely removed and note replaced with anything else. So I'm just wondering.

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Aug 17, 2015

Member

The first line was just a filter making sure all version defines were present. Removing it removes the check.

Member

binary1248 commented Aug 17, 2015

The first line was just a filter making sure all version defines were present. Removing it removes the check.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Aug 17, 2015

Member

Removing it removes the check.

And is that okay or not?

Member

eXpl0it3r commented Aug 17, 2015

Removing it removes the check.

And is that okay or not?

@binary1248

This comment has been minimized.

Show comment
Hide comment
@binary1248

binary1248 Aug 17, 2015

Member

Well... it works, but it would allow bogus config files without a major or minor version also slip through. 😛

Member

binary1248 commented Aug 17, 2015

Well... it works, but it would allow bogus config files without a major or minor version also slip through. 😛

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Aug 19, 2015

Member

Well if nobody has anything against this PR and it's implications as see above in the next days I'll finally merge it.

Member

eXpl0it3r commented Aug 19, 2015

Well if nobody has anything against this PR and it's implications as see above in the next days I'll finally merge it.

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Aug 20, 2015

Member

I won't repeat myself again. :D 👍

Member

TankOs commented Aug 20, 2015

I won't repeat myself again. :D 👍

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Aug 22, 2015

Member

Merged in 110feeb on branch 2.3.x.

Also created #950 so we can keep track of the issue and implement a "proper" fix at a later point.

Member

eXpl0it3r commented Aug 22, 2015

Merged in 110feeb on branch 2.3.x.

Also created #950 so we can keep track of the issue and implement a "proper" fix at a later point.

@eXpl0it3r eXpl0it3r closed this Aug 22, 2015

@TankOs

This comment has been minimized.

Show comment
Hide comment
@TankOs

TankOs Aug 27, 2015

Member

Thanks!

Member

TankOs commented Aug 27, 2015

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment