-
Notifications
You must be signed in to change notification settings - Fork 14
Cmake #295
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: master
Are you sure you want to change the base?
Cmake #295
Conversation
|
In theory these PR's are ready. But since I'm updating to the latest versions (we have updated to a different version internally) I'd like to merge everything together and do a final build... |
| @@ -1,7 +1,7 @@ | |||
| inherit: [cpackage, ninja, install, licenses] | |||
| inherit: [cpackage, ninja, install, licenses, make] | |||
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 afraid that doesn't work this way. After merging #282, the ninja and make classes set different values for the jobServer key. 😕
The only way forward that I see is to move the common parts of this class to classes/basement/bits/cmake.yaml and create a new class cmake-make that does inherit: ["basement::bits::cmake", make].
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.
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.
This seems fragile. I just had another idea. We could set jobServer: "fifo" explicitly in the cmake class again with a big, fat comment explaining why. This mode is compatible with both tools...
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.
If make understands fifo as well why do we not simply set jobServer: "fifo" in the make-class?
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.
It will fail if you start Bob from an old make. See here. OTOH, as soon as you hit a cmake/ninja recipe, this will happen anyway. Looking back, it should probably be a warning and just not pass the incompatible job server instead of failing.
Having said that, it sounds like an artificial problem. So using "fifo" consistently looks like the best way forward. Let's do it this way...
|
|
||
| metaEnvironment: | ||
| PKG_VERSION: "4.2.1" | ||
| PKG_VERSION: "${DEVEL__CMAKE_VERSION:-4.2.1}" |
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 not a huge fan of this to be honest. It's better to have a dedicated cmake-3 recipe.
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.
Thanks. I guess you overlooked the remaining DEVEL__CMAKE_VERSION override...
classes/compat/cmake-3.31.yaml
Outdated
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.
Not sure if the compat namespace makes sense here. Name it classes/cmake-3.yaml?
To support the Makefile-Generator of cmake we have to use the same jobServer implementation for ninja and make as both classes are inherited by cmake. See: BobBuildTool#295 (comment)
Some builds are failing if ninja is used while the succeed with make. Make the Generator setable and support 'Unix Makefiles' next to ninja.
CMake 4.0 removed support for cmake_minium_version <= 3.5 ([1]). Since there are still packages around requiring older cmake versions add a cmake-3 class to be used by them. [1] https://cmake.org/cmake/help/latest/command/cmake_minimum_required.html
|
|
||
| metaEnvironment: | ||
| PKG_VERSION: "4.2.1" | ||
| PKG_VERSION: "${DEVEL__CMAKE_VERSION:-4.2.1}" |
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.
Thanks. I guess you overlooked the remaining DEVEL__CMAKE_VERSION override...
|
|
||
| provideDeps: [ "*-tgt" ] | ||
|
|
||
| bootstrap: |
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.
AFAICT, the bootstrap package can be removed, couldn't it?
|
|
||
| checkoutDeterministic: True | ||
| checkoutScript: | | ||
| patchApplySeries $<@cmake/*.patch@> |
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.
This is certainly not working, or? Even if it does currently, it will fail in the future. We have to keep the cmake-3 patches separate...
No description provided.