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

Clean up CMake build #1964

Merged
merged 9 commits into from
Jan 8, 2025
Merged

Clean up CMake build #1964

merged 9 commits into from
Jan 8, 2025

Conversation

Gold856
Copy link
Contributor

@Gold856 Gold856 commented Jan 7, 2025

This increases the use of more standard CMake variables for controlling things like PIC, output directories, and MSVC flags, and also removes redundancies like empty prefixes on Windows. This also sets all CMake policies up to 3.29 to NEW, which allows certain MSVC flags to be controlled by variables instead, and removes /W3 from the default set of flags, which reduces the number of warnings of compile flags being overriden to about 2.

@Gold856 Gold856 changed the title Cmake Clean up CMake build Jan 7, 2025
Generator expressions were used to prevent the addition of /MD and /Zi if the CMake version is new enough to have specific MSVC variables for controlling those flags
MSVC flag is not needed and the minimum CMake version is higher now
It's not a valid flag for static linking
@dellaert dellaert requested a review from jlblancoc January 7, 2025 14:39
@dellaert
Copy link
Member

dellaert commented Jan 7, 2025

Oooh, nice! I am not a cmake guru, but @jlblancoc is :-) and he might also have visibility on effect on ROS (if any). I requested a review from him to look over this. The CI will also tell us a lot.

@dellaert
Copy link
Member

dellaert commented Jan 7, 2025

PS, @Gold856, not a deal-breaker, but I am more comfortable with contributors that have their name and affiliation listed. We can then also properly acknowledge you in a next release.

Copy link
Member

@jlblancoc jlblancoc left a comment

Choose a reason for hiding this comment

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

All changes look awesome to me!

@Gold856 is the new CMake guru in town ;-)

@jlblancoc
Copy link
Member

@dellaert ROS builds shouldn't be affected (but I'll keep an eye on the build farm).
All CI passed here, so you can merge it.

@varunagrawal varunagrawal merged commit 29d0db2 into borglab:develop Jan 8, 2025
33 checks passed
@Gold856 Gold856 deleted the cmake branch January 9, 2025 22:17
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