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

Update driver install dir to follow cmake install prefix #1792

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

jkohnert
Copy link
Contributor

When defining GDL_LIB_DIR, the install process does not take into account a set CMAKE_INSTALL_PREFIX. I implemented this patch as part of the Arch-Linux AUR package, since having *.so files in /usr/share/ is considered bad packaging by the Arch-Linux guidlines.

Once applied, setting GDL_LIB_DIR is compatible to GDL_DATA_DIR (f.e. lib/gnudatalanguage, and share/gnudatalanguage), and both locations will respect a set CMAKE_INSTALL_PREFIX (f.e /usr/), so the install dirs will be f.e. /usr/lib/gnudatalanguage, and /usr/share/gnudatalanguage, respectively.

@jkohnert
Copy link
Contributor Author

There seem to be build/test issues, I'll look into this.

@jkohnert jkohnert marked this pull request as draft March 29, 2024 17:42
@jkohnert
Copy link
Contributor Author

I've now reworked this whole thing.

The basic idea is to always use the configurable $INSTALL_PREFIX/lib/gnudatalanguage as driver install dir. We determine the location of installed *.pro-files (and some other things) as being installed in locations we can get from the executable path if installed in some bin directory. The same logic should be applied for drivers as well.

I added some CMake statements to also be able to use built drivers from the build-dir directly in tests by copying the *.driver_info files to the build-location if we set GDL_DRV_DIR.

The logic should work for Gnu-install-dir-aware installations as well es MacOS, or Windows.

I included some Clang-Tidy related changes as well. 😇

BTW: Are there any plans to include C++17, or C++20 dependent code? The return type of lib::PathSeparator() and alike could possibly be changed to constexpr std::string_view or something like that, but that would require C++17, at least.

Best, Jan

@jkohnert jkohnert marked this pull request as ready for review March 31, 2024 22:07
@GillesDuvert
Copy link
Contributor

OMG, many thanks @jkohnert !
However, the few graphic tests do not pass (on all platforms).

@jkohnert
Copy link
Contributor Author

jkohnert commented Apr 8, 2024

@GillesDuvert thanks for having a look at this. The failing tests were obviously unintended and I actually thought, I had fixed the problem. I'll have another look as soon as I have time.

@GillesDuvert GillesDuvert added todo-test WIP Work In Progress: PR acceptance will be considered when this label is removed. labels Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
todo-test WIP Work In Progress: PR acceptance will be considered when this label is removed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants