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

Modernize usage of CMake #79

Merged
merged 3 commits into from
Jan 8, 2025
Merged

Conversation

bbockelm
Copy link
Collaborator

@bbockelm bbockelm commented Jan 4, 2025

A bit of spring cleaning for how we invoke CMake.

@bbockelm bbockelm force-pushed the modernize_cmake branch 2 times, most recently from d2fc683 to 5724d8e Compare January 4, 2025 15:11
- Use targets instead of environment variables where possible
- Use XRootD's find_package over local one
- Remove compatibility macros with very old CMake
- Add improved std::filesystem detection
- Always run with `-Werror` in Debug mode
@bbockelm
Copy link
Collaborator Author

bbockelm commented Jan 6, 2025

@jhiemstrawisc - any interest in reviewing this?

I feel like there's enough CMake arcana here that I might be the only one who's comfortable working in it :/

@jhiemstrawisc
Copy link
Member

At the very least, I'll take a peak to see if I can learn anything 🤓 A quick glance tells me I probably won't have meaningful comments, though.

@jhiemstrawisc jhiemstrawisc self-requested a review January 8, 2025 21:09
Copy link
Member

@jhiemstrawisc jhiemstrawisc left a comment

Choose a reason for hiding this comment

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

I was able to take a look and maybe learned a few things. Approving so you can self-review and merge when ready.

@bbockelm
Copy link
Collaborator Author

bbockelm commented Jan 8, 2025

@jhiemstrawisc - one thing of note is that, with this refactor, I've started to dig into the automatic dependency resolution / downloads for CMake.

This would allow us to bootstrap the build from nothing - eg, if you don't have XRootD, CMake could install and configure it from scratch.

(That's not here but there's potential for the future!)

@bbockelm bbockelm merged commit e945eaf into PelicanPlatform:main Jan 8, 2025
3 checks passed
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.

2 participants