Skip to content
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

Place bundled SFML include paths before others #841

Merged
merged 1 commit into from Sep 6, 2014
Merged

Place bundled SFML include paths before others #841

merged 1 commit into from Sep 6, 2014

Conversation

AMDmi3
Copy link
Contributor

@AMDmi3 AMDmi3 commented Aug 20, 2014

This allows to build with bundled SFML when system SFML (of another version) is installed

This allows to build with bundled SFML when system SFML (of another
version) is installed
@delroth
Copy link
Member

delroth commented Aug 23, 2014

This branch should only be used if system SFML is not found. What is this trying to fix?

@badkarma12
Copy link

Delroth, Should not it also be used if you have an installed version less than the current, but greater than 1.x? I.E. version 2.0 instead of 2.1? I don't think the version check was ever updated when 2.1 was released, because right now (from the comment) it looks like cmake is just looking for ANY major version. I could be wrong (as per usual), but this would make the dolphin version (more reliably updated than individuals versions) without constantly having to update the version check.

@shuffle2
Copy link
Contributor

I think 2.0 Has bugs which we had to work around in our version in
Externals, which has since been fixed in mainline. (Or else that was for
some 1.x versions, I forget). In any case it's nice to be able to sanely
depend on a specific version.

On Sat, Aug 23, 2014 at 4:06 PM, badkarma12 notifications@github.com
wrote:

Should not it also be used if you have an installed version less than the
current, but greater than 1.x? I.E. version 2.0 instead of 2.1? I don't
think the version check was ever updated when 2.1 was released.


Reply to this email directly or view it on GitHub
#841 (comment).

@AMDmi3
Copy link
Contributor Author

AMDmi3 commented Aug 25, 2014

Well, the fact that local includes must come before system ones should be quite enough to fix it.

In my specific case, there are two problems:

  1. Dolphin 4.0.2 doesn't build with SFML 2.1 which is installed on FreeBSD by default. It correctly choses to use bundled version, but it picks up includes from system one so the build breaks.
  2. Even if dolphin is made to build, SFML doesn't care not only about compatibility between branches but also about an ability to install different branches simultaneously, so (as there's both software which uses sfml1 and sfml2) maintainers have to e.g. install specific sfml branch under a different prefix, and that prefix may be added to cmake flags, so again even if dolphin choses not to use this version, it'll break the build.

That said, it's much less painful for me to patch the CMakeLists.txt to unconditionally use bundled sfml. For all described use cases, this change is required.

@shuffle2
Copy link
Contributor

shuffle2 commented Sep 3, 2014

  1. Dolphin 4.0.2 doesn't build with SFML 2.1 which is installed on FreeBSD by default. It correctly choses to use bundled version, but it picks up includes from system one so the build breaks.

This seems like a good enough reason... @delroth , @Sonicadvance1 , <linux ppl>, merge it?

@Sonicadvance1
Copy link
Contributor

I have SFML 2.1 installed here(Ubuntu 14.04) and Dolphin correctly uses the version from the externals and compiles without issue.
I have tried both 4.0.2 and and version of Dolphin from today to compile.
I don't feel great merging this without understanding why it works for me but not you, even if we are on different distros

@archshift
Copy link
Contributor

While the buildbot succeeded, all warnings need to be fixed with this in order not to break master, which uses -Werror.

@shuffle2
Copy link
Contributor

shuffle2 commented Sep 3, 2014

Sorry @archshift I was a bit confused where the log was coming from (thought it was a different PR as the windows buildbot was acting a bit strange at the time)...this is just a cmake change so it wouldn't need such care.

@AMDmi3
Copy link
Contributor Author

AMDmi3 commented Sep 3, 2014

I don't feel great merging this without understanding why it works for me but not you, even if we are on different distros

As I've said, the change should be merged ragardless - local include paths must always come before system ones. Regarding the build failure with SFML 2.1, I can open another issue. Maybe that's related to clang which is used on newer versions of FreeBSD instead of gcc.

@Sonicadvance1
Copy link
Contributor

Interesting. Alright.

Sonicadvance1 added a commit that referenced this pull request Sep 6, 2014
Place bundled SFML include paths before others
@Sonicadvance1 Sonicadvance1 merged commit 7a1eca1 into dolphin-emu:master Sep 6, 2014
@AMDmi3 AMDmi3 deleted the bundled-smfl-include-order branch September 7, 2014 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
6 participants