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

Do not install libfmt by default #5808

Merged
merged 1 commit into from
Apr 27, 2024

Conversation

orhun
Copy link
Contributor

@orhun orhun commented Mar 25, 2024

closes #5805

Copy link
Member

@Web-eWorks Web-eWorks left a comment

Choose a reason for hiding this comment

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

Modifying the CMakeLists.txt provided by libfmt means this change will be overwritten every time libfmt is updated. I'd strongly recommend you set(FMT_INSTALL OFF CACHE) the variable (possibly using FORCE) from the top-level CMakeLists.txt before the add_subdirectory(contrib/fmt) call.

@Web-eWorks
Copy link
Member

Thank you for taking the time to address this issue!

@orhun
Copy link
Contributor Author

orhun commented Mar 26, 2024

Made the recommended change in c737816

Not sure if we still need to keep the option. Let me know how it looks!

@Web-eWorks
Copy link
Member

I'll try to test this later today, but there should be no need to keep the modification to contrib/fmt/CMakeLists.txt.

@pcercuei
Copy link
Contributor

pcercuei commented Apr 3, 2024

@orhun if you git rebase -i origin/master, it will open a text editor, in which you can remove the "do not install libfmt by default" commit, and the revert commit, and only keeping the "Set FMT_INSTALL..." commit. Once you have that, you can force-push your PR with git push -f.

@orhun orhun force-pushed the chore/disable_fmt_install branch from b82a940 to efb09fc Compare April 3, 2024 21:59
@orhun orhun requested a review from Web-eWorks April 3, 2024 21:59
@orhun
Copy link
Contributor Author

orhun commented Apr 19, 2024

I'm not sure why the CI fails.

@pcercuei
Copy link
Contributor

@orhun well it fails locally too. This is not the right syntax, it's missing a description string. This works:

set(FMT_INSTALL OFF CACHE BOOL "Enable install of libfmt" FORCE)

@orhun orhun force-pushed the chore/disable_fmt_install branch from efb09fc to a2ce759 Compare April 20, 2024 08:33
@orhun
Copy link
Contributor Author

orhun commented Apr 20, 2024

Updated, thanks!

@Web-eWorks Web-eWorks merged commit 2888d38 into pioneerspacesim:master Apr 27, 2024
5 checks passed
@Web-eWorks
Copy link
Member

Thanks for the patience! Tested locally and it all looks good.

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.

cmake tries to install libfmt
3 participants