-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[build-script] Improvements to CMakeOptions. #23303
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
[build-script] Improvements to CMakeOptions. #23303
Conversation
This looks good to me, but, I think that I would defer to @Rostepher or @ddunbar here. |
b2a5491
to
45d2137
Compare
45d2137
to
ce2463d
Compare
Rebased on top of current master. @swift-ci please test |
Build failed |
Build failed |
ce2463d
to
9ef97f0
Compare
Fix a problem where the version parameters are neither bools, numbers or strings (they get parsed into specific types by the argparser), and cannot be passed directly to (The Linux problem seemed unrelated, though) @swift-ci please test |
Build failed |
Build failed |
9ef97f0
to
632f936
Compare
Making the linter happy. @swift-ci please test |
Build failed |
Build failed |
632f936
to
1170c57
Compare
Recovering the changes overwritten in the last forced push. @swift-ci please test |
Build failed |
Build failed |
CMakeOptions was briefly used in the cmake module, but mostly unused in the rest of the modules, however, several products were dealing with what eventually will end as CMake options, but not using the already existing code, and duplicating it. The changes tries to unify all usage around CMakeOptions which avoids code duplication. Additionally, it provides some more API surface in CMakeOptions to make it more useful. - The initializer can be passed a list of tuples or another CMakeOptions to copy into the newly created options. - Boolean Python values are treated automatically as boolean CMake options and transformed into `TRUE` and `FALSE` automatically. - Provides `extend`, which will add all the tuples from a list or all the options from another `CMakeOptions` into the receiving `CMakeOptions`. - Provides `__contains__`, which makes checking for existing options a little easier (however, being `CMakeOptions` basically a list, the checking has to include also the value, which is not that helpful). - Modify LLVM and Swift products to use `CMakeOptions`. It makes the boolean values clearer and avoid a lot of repetitive string formatting. - Modify the tests to make them pass and provide new test for newly introduced features.
1170c57
to
6bf0826
Compare
Ready for review. Fixed the conflicts with current master, and checked it still works as intended. @swift-ci please test |
@Rostepher: I think this would be valuable going forward, and might simplify the later changes. If there's some feedback (positive or negative) I can also work on it. If we don’t want the changes at all, it would also be nice to know, to try to work around them, and not be hold by them. Thanks a lot. |
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.
These changes look good to me.
Applies to SR-237
This code was extracted and improved from #23038.
CMakeOptions was briefly used in the cmake module, but mostly unused in
the rest of the modules, however, several products were dealing with
what eventually will end as CMake options, but not using the already
existing code, and duplicating it.
The changes tries to unify all usage around CMakeOptions which avoids
code duplication. Additionally, it provides some more API surface in
CMakeOptions to make it more useful.
to copy into the newly created options.
options and transformed into
TRUE
andFALSE
automatically.extend
, which will add all the tuples from a list or allthe options from another
CMakeOptions
into the receivingCMakeOptions
.__contains__
, which makes checking for existing options alittle easier (however, being
CMakeOptions
basically a list, thechecking has to include also the value, which is not that helpful).
CMakeOptions
. It makes theboolean values clearer and avoid a lot of repetitive string
formatting.
introduced features.
/cc @Rostepher, @compnerd