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

fix installing via cmake #374

Merged
merged 4 commits into from
Oct 14, 2024
Merged

fix installing via cmake #374

merged 4 commits into from
Oct 14, 2024

Conversation

Arniiiii
Copy link
Contributor

What was the problem?

https://bugs.gentoo.org/933479
On Gentoo, a multilib library was looking for magic_enum's CMake config files, but couldn't find, because they are in ABI-dependant folder.

magic_enum's CMake script installs CMakeConfig related stuff to /usr/lib64 or /usr/lib .
But:

  • The library is header-only
    • Therefore it is NOT ABI-dependant.

The pull request does following things:

  1. Make CMake install CMake's config files and pkgconfig to ABI-independant folder such as /usr/share/magic_enum/cmake/... and /usr/share/magic_enum/magic_enum.pc .
  2. Allows to check via tests if installed version via magic_enum's CMake config files is usable.
  3. Allows to check via tests if installed version via magic_enum's pkgconfig file is usable.
  4. Adds a github workflow to check if installed version via CMake and via pkgconfig are usable at least on a latest Ubuntu machine.
  5. Adds .cmake-format file. It's a file for usage of cmake-format program. The file specifies what style to use for CMakeLists.txt and *.cmake files. I haven't applied for all such files, since I've touched not all of them.
  6. Adds some convinient folders to .gitignore

microfix

add gh workflow part for pkgconfig

uncomment that should not be commented

move some add_subdirectory to logically correct place
@Neargye
Copy link
Owner

Neargye commented Sep 19, 2024

Hi,
Thanks for RP
could you have time look at these problems #377 #318 and perhaps add them to the pr

@Arniiiii
Copy link
Contributor Author

Arniiiii commented Sep 19, 2024

Hi, Thanks for RP could you have time look at these problems #377 #318 and perhaps add them to the pr

I'll try at Saturday.

But from first look:
#318 not sure it can be solved because I couldn't get such behaviour ah, I read badly. ok, seems feasible
#377 seams feasible

@Neargye Neargye added this to the v0.9.7 milestone Sep 19, 2024
@Arniiiii
Copy link
Contributor Author

Arniiiii commented Sep 21, 2024

Ok, now, with new commit we have:

  1. Now headers can be accessible only via magic_enum/magic_enum.hpp.
    1. Why?
    1. Now tests, examples and some code at module/magic_enum.cppm include the headers via i.e. #include <magic_enum/magic_enum.hpp>
    2. I don't know about Bazel, but for me it seems changing in BUILD.bazel from includes = ["include/magic_enum"], to includes = ["include"], would be appropriate with the change.
    3. UPD: Same for Meson.
  2. Add a test script at test_installed_version.bash with literally same logic as .github/workflows/ubuntu_test_installed_version.yml . Also added Dockerfile and .dockerignore files with which I tested if installation works correctly. If you want, I wiil delete these two files.
  3. Improved logic of finding pkgconfig files in test/CMakeLists.txt if MAGIC_ENUM_OPT_TEST_INSTALLED_VERSION_PKGCONFIG is enabled
    • Now it also try to check /usr/local/ for pkgconfig
    • It gives an explanation as a warning.
  4. In .github/workflows/ubuntu_test_installed_version.yml now instead of -j4 it uses -j$(nproc) .
  5. A little bit formatting of main CMakeLists.txt.

@JWCS
Copy link

JWCS commented Oct 4, 2024

For what it's worth, I tested @Arniiiii 's branch, with these latest changes, and it worked for me. Looking forward to next release.

@Neargye Neargye merged commit a72a053 into Neargye:master Oct 14, 2024
19 checks passed
@Neargye
Copy link
Owner

Neargye commented Oct 14, 2024

@Arniiiii Thanks for changes, all looks good. Include via <magic_enum/magic_enum.hpp> it's logical and understandable.

@Neargye
Copy link
Owner

Neargye commented Oct 14, 2024

I'll still try to adjust the formatting to improve such moments

a local magic_enum." NO

@jcar87
Copy link

jcar87 commented Nov 19, 2024

@Arniiiii Thanks for changes, all looks good. Include via <magic_enum/magic_enum.hpp> it's logical and understandable.

This change does make perfect sense (a subfolder rather than at the root if the include folder, and aligning the behaviour of an installed location with add_subdirectory) but please me mindful that changes like this disrupt users who were already using the library as they may need to change their source files.

This very change was accidentally introduced, then reverted in #310 (comment) - and now it's being brought back again.

Chiming in from Conan Center - we just package the version as is, so that's not a problem at all. But had to spend some additional time finding an authoritative answer as to how the header should be included. I would probably suggest making it very clear in the README what the #include should be, and perhaps add a comment to #310, since the behaviour of a cmake install changed twice.

@Neargye
Copy link
Owner

Neargye commented Nov 19, 2024

@jcar87 I agree, my bad.
I will update the documentation to clarify.

@Neargye Neargye mentioned this pull request Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants