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

make ARCH_INDEPENDENT libraries to have CMake configs installed to share #42

Merged
merged 10 commits into from
Sep 2, 2024

Conversation

Arniiiii
Copy link
Contributor

overall right change

maybe fixes #30 if I understand the guys problem correctly

Copy link
Owner

@TheLartians TheLartians left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the PR, it looks great! just so I understand correctly, with this architecture-independent projects will be installed to CMAKE_INSTALL_DATADIR, which is usually /usr/share instead of the standard platform-dependent directory like /usr/lib, correct?

Also should we add a test project to ensure this works as expected and doesn't break in the future?

CMakeLists.txt Outdated Show resolved Hide resolved
added test for header only, though it works only in github workflow

added some folders to gitignore

made github workflow to install not on a system but in a folder `install_dir`

formatted some CMakeLists.txt with `cmake-format`
@Arniiiii
Copy link
Contributor Author

@TheLartians Could you check it now, please?

@Arniiiii
Copy link
Contributor Author

Thanks a lot for the PR, it looks great! just so I understand correctly, with this architecture-independent projects will be installed to CMAKE_INSTALL_DATADIR, which is usually /usr/share instead of the standard platform-dependent directory like /usr/lib, correct?

Yep. For example, check this bug: https://bugs.gentoo.org/show_bug.cgi?id=933479

Also should we add a test project to ensure this works as expected and doesn't break in the future?

Added, check please

Copy link
Owner

@TheLartians TheLartians left a comment

Choose a reason for hiding this comment

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

Thanks for the updates and adding a test case!

The main code changes look good, but there are some unrelated changes in the .gitignore and unit testing, the latter apparently failing the CI run. Could you undo or address these changes?

.github/workflows/test.yml Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
@Arniiiii
Copy link
Contributor Author

Arniiiii commented Sep 2, 2024

@TheLartians ping

@Arniiiii Arniiiii requested a review from TheLartians September 2, 2024 10:55
Copy link
Owner

@TheLartians TheLartians left a comment

Choose a reason for hiding this comment

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

Looks great, thanks a lot for adding the changes and the test case! Also sorry for the long response times, currently don't have a lot of time for open-source work.

@TheLartians TheLartians merged commit 5f51898 into TheLartians:master Sep 2, 2024
3 checks passed
@TheLartians
Copy link
Owner

Feature published in v1.12.0 🎉

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.

Arch-independent library directory support
2 participants