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] Argument Builder DSL Conversion: Episode 1 #13117
Conversation
@swift-ci please test |
@swift-ci please smoke test |
Build failed |
I added a bunch of args in #12955 (and related PRs) that will have to move over to this DSL format |
Build failed |
@swift-ci please test |
@swift-ci please smoke test |
Build failed |
Build failed |
@swift-ci please test Mac OS X |
@swift-ci please clean test Mac OS X |
@swift-ci please test Mac OS X |
@swift-ci please test OS X |
Build failed |
option(['-e', '--eclipse'], store('cmake_generator'), | ||
const='Eclipse CDT4 - Ninja', | ||
default=defaults.CMAKE_GENERATOR, | ||
help="use CMake's Xcode generator (%(default)s by default)") |
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 string seems accidentally repeated.
Overall very cool. 👏
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.
@swift-ci please test |
@swift-ci please smoke test |
Build failed |
Build failed |
…ppease the flake8 gods.
8ad0f98
to
38ac230
Compare
@swift-ci please test |
@swift-ci please smoke test |
Build failed |
Build failed |
with mutually_exclusive_group(): | ||
set_defaults(assertions=True) | ||
|
||
# TODO: Convert to store_true |
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.
Why do we need to use "store_true" here?
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.
Because using store_const
and then setting the const to True
is a bit ridiculous, since that's almost store_true
, just without the safety of setting the default to False
. It doesn't make sense to store a constant boolean for an option, just use store_true
, store_false
, toggle_true
and toggle_false
instead.
builder = parser.to_builder() | ||
|
||
# Prepare DSL functions | ||
option = builder.add_option |
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.
Why change this to "option" instead of using "add_option"?
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.
Calling it just option
is shorter which cuts down on visual noise and it's more declarative inline with the rest of the DSL.
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
* Imported the new argparse overlay module and added the setup code for the DSL. * Converted the CMake generator flags to use the new builder DSL. * Converted the assertions argument group to use the new builder DSL. * Converted the LLVM-specific settings argument group to use the new builder DSL. * Converted the Android build settings argument group to use the new builder DSL. * Removed unused action aliases from the builder DSL setup section to appease the flake8 gods. * Fixed small typo in help message for --eclipse option.
Purpose
This PR begins the conversion to the new argument builder DSL. There's no real functional changes, just converting to the more expressive builder system and adding some visual separators between argument groups. More conversion PRs are to follow until all the arguments are converted.
rdar://34336890