Neargye / magic_enum

Static reflection for enums (to string, from string, iteration) for modern C++, work with any enum type without any macro or boilerplate code
MIT License
4.99k stars 445 forks source link

fix installing via cmake #374

Closed Arniiiii closed 1 month ago

Arniiiii commented 3 months ago

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 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
Neargye commented 2 months ago

Hi, Thanks for RP could you have time look at these problems https://github.com/Neargye/magic_enum/issues/377 https://github.com/Neargye/magic_enum/issues/318 and perhaps add them to the pr

Arniiiii commented 2 months ago

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: https://github.com/Neargye/magic_enum/issues/318 not sure it can be solved because I couldn't get such behaviour ah, I read badly. ok, seems feasible https://github.com/Neargye/magic_enum/issues/377 seams feasible

Arniiiii commented 2 months ago

Ok, now, with new commit we have:

  1. Now headers can be accessible only via magic_enum/magic_enum.hpp.
    1. Why?
      • Because we want consistency of installed version and cmake-add_subdirectory version.
      • And it seems that a guy from #377 and esteemed ClausKlein from #318 want it to be either all headers installed in a folder or just main header and others in a folder. Second behaviour means a lot of hassle: or we need to change position of include/magic_enum/magic_enum.hpp to include/magic_enum.hpp , or we got some problems. Though, I believe if you, @Neargye think that the option "main header + others in a folder" is better, maybe just placing it one directory above and fixing some #include directives would be enough.
    2. Now tests, examples and some code at module/magic_enum.cppm include the headers via i.e. #include <magic_enum/magic_enum.hpp>
    3. 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.
    4. 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 commented 1 month ago

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 commented 1 month ago

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

Neargye commented 1 month ago

I'll still try to adjust the formatting to improve such moments https://github.com/Neargye/magic_enum/blob/a72a0536c716fdef4f029fb43e1fd7e7b3d9ac9b/CMakeLists.txt#L31

jcar87 commented 1 week ago

@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 https://github.com/Neargye/magic_enum/issues/310#issuecomment-1808234766 - 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 https://github.com/Neargye/magic_enum/issues/310, since the behaviour of a cmake install changed twice.

Neargye commented 1 week ago

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