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

Use GNUInstallDirs module for cmake install paths #1576

Merged
merged 1 commit into from Sep 1, 2019
Merged

Use GNUInstallDirs module for cmake install paths #1576

merged 1 commit into from Sep 1, 2019

Conversation

JonnyPtn
Copy link
Contributor

@JonnyPtn JonnyPtn commented Apr 9, 2019

  • Has this change been discussed on the forum or in an issue before?
  • Does the code follow the SFML Code Style Guide?
  • Have you provided some example/test code for your changes?
  • If you have additional steps which need to be performed list them as tasks!

Description

This replaces our custom install paths with the ones from the cmake GnuInstallDirs module

This PR is related to the issue #1563

Tasks

  • Tested on Linux
  • Tested on Windows
  • Tested on macOS
  • Tested on iOS
  • Tested on Android

How to test this PR?

Build and install SFML (preferably to a custom CMAKE_INSTALL_PREFIX) and check the directory structure.

Seems I'm not able to request reviewers, but @eliasdaler , @fxcoudert and @Ceylo would be great to take a look

@eXpl0it3r eXpl0it3r added this to Discussion in SFML 2.6.0 via automation Apr 9, 2019
@eXpl0it3r eXpl0it3r added this to the 2.6 milestone Apr 9, 2019
@eXpl0it3r eXpl0it3r self-assigned this Apr 9, 2019
@eXpl0it3r eXpl0it3r moved this from Discussion to Review & Testing in SFML 2.6.0 Apr 9, 2019
Copy link
Member

@eXpl0it3r eXpl0it3r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beyond the example directory thingy, this looks good. Tested it on Windows 10 and Debian 9.

cmake/Macros.cmake Outdated Show resolved Hide resolved
@eXpl0it3r
Copy link
Member

@jcowgill can you give feedback on this, especially w.r.t. pkgconfig?

CMakeLists.txt Outdated Show resolved Hide resolved
@jcowgill
Copy link
Contributor

Presumably this section of Macros.cmake needs updating?

    # add the install rule
    install(TARGETS ${target} EXPORT SFMLConfigExport
            RUNTIME DESTINATION bin COMPONENT bin
            LIBRARY DESTINATION lib${LIB_SUFFIX} COMPONENT bin
            ARCHIVE DESTINATION lib${LIB_SUFFIX} COMPONENT devel
            FRAMEWORK DESTINATION "." COMPONENT bin)

@jcowgill
Copy link
Contributor

And I expect to do this properly, all occurrences of lib${LIB_SUFFIX} should be replaced with ${CMAKE_INSTALL_LIBDIR}

@jcowgill
Copy link
Contributor

And for pkg-config specifically, ${CMAKE_INSTALL_LIBDIR}/pkgconfig is the correct directory to use by default (but keep the extra override for BSD systems).

@JonnyPtn
Copy link
Contributor Author

Thanks for the feedback @jcowgill, think I've covered them all now?

Copy link
Contributor

@eliasdaler eliasdaler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, it looks good. I'll test on Linux soon.

I feel like we need to summarize somewhere which variables from GNUInstallDirs are used, because previously we had variables which were visible at configuration step, but now users might not know which variables control where stuff gets installed.

CMakeLists.txt Outdated Show resolved Hide resolved
cmake/Macros.cmake Show resolved Hide resolved
@JonnyPtn
Copy link
Contributor Author

Not sure how much benefit there is to us explaining these variables, and it's kind of out of SFML scope. Realistically the user should only need to worry about CMAKE_INSTALL_PREFIX, and anything else is more advanced usage which they can probably work out on their own.

I think if we add a comment linking to the cmake docs this should suffice, but I'm happy to add more if that's the decision

@JonnyPtn JonnyPtn changed the title Use GnuInstallDirs module for cmake install paths Use GNUInstallDirs module for cmake install paths Apr 15, 2019
@eliasdaler
Copy link
Contributor

Realistically the user should only need to worry about CMAKE_INSTALL_PREFIX, and anything else is more advanced usage which they can probably work out on their own.

Yeah, agreed. Okay, it's good enough for now. 👍

@JonnyPtn
Copy link
Contributor Author

Have just noticed this will break the xcode templates which rely on SFML_DEPENDENCIES_INSTALL_PREFIX to copy the frameworks into the app bundle...

I can probably change it to use the generator expression for the output directory of SFML if that's suitable?

@eXpl0it3r
Copy link
Member

Sounds good to me, even though I don't fully understand the implications.

Looks like the Android build fails, do we have to adjust builbot as well?

@jcowgill
Copy link
Contributor

There are still a few instances of LIB_SUFFIX left in the pkg-config files in tools/pkg-config/. It's also set once in the Android code in CMakeLists.txt, but I'm not sure what to do about that though.

@JonnyPtn
Copy link
Contributor Author

I think that's the pkg config stuff sorted @jcowgill?

For the xcode template, even after updating the paths in the post build step it still fails to link for me, and I don't see how it would work as it doesn't set the library search paths as far as I can tell. I also haven't managed to get it working in the latest master anyway, so I'm inclined to say this is a separate issue to discuss

Copy link
Contributor

@jcowgill jcowgill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think all the pkg-config stuff is sorted now. Looks good on the Linux side. 👍

@eXpl0it3r eXpl0it3r moved this from Review & Testing to Ready in SFML 2.6.0 Jul 1, 2019
@eXpl0it3r eXpl0it3r merged commit 27a4c83 into SFML:master Sep 1, 2019
SFML 2.6.0 automation moved this from Ready to Done Sep 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
SFML 2.6.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants