-
Notifications
You must be signed in to change notification settings - Fork 24
Set CMAKE_PROJECT_TOP_LEVEL_INCLUDES in base preset #142
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
base: main
Are you sure you want to change the base?
Conversation
Set CMAKE_PROJECT_TOP_LEVEL_INCLUDES to our GitHub dependency provider as a common default so that common out of the box workflows will contintue to work.
I have entertained this idea with @bretbrownjr . You can read our discussion here: #105 (comment) and #105 (comment) . I think this is a good short-term patch, but I think we might need to essentially redesign presets to fit current combination explosion. I am in support of this change, but I will leave the decision to @bretbrownjr and @camio . |
I saw @camio commenting earlier that we broke his workflow, and I think this minimal solution is OK for out of the box use for someone kicking the tires, or in one of our common cases. Presets just aren't designed to support all the possible combinations, though. They're for a small-ish set for a group who builds things in a very consistent way, sharing the same workflow, with a small set of tools involved. That is a very common situation. But it's really not for the matrix explosion of a public C++ project where you want to check all the things all the ways. |
We might be able to use |
Except FetchContent out of the box also breaks other workflows. I started this packaging effort a few months ago because my out of the box experience wasn't working. I'm not opposed to some branded workflows obviously supporting FetchContent. I don't agree that it's a (singular) sensible default though. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to discuss this more.
I am not opposed to presets in this direction. I'm not convinced it goes in the root preset however.
The reasons Bloomberg has been slow to adopt presets have more to do with the design limitations of presets than any in-house tech conflicts. Namely:
|
I'm supportive of this PR. There are tradeoffs for whatever default we choose, but a FetchContent default has a fully known dependency set which makes issues at first-exposure (like #141) much less likely. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm approving this commit for merge since it fixes the regression documented in #141, but we can continue building consensus around what we want our default ultimately to be.
I've also approved for at least the short term due to the regression. @bretbrownjr are we ok to fix and open another issue to re-evaluate? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
I'm also OK to merge this right now and do follow-up PRs.
Set CMAKE_PROJECT_TOP_LEVEL_INCLUDES to our GitHub dependency provider as a common default so that common out of the box workflows will contintue to work.
Edit from river: fix #141