[build-script] Argument Builder DSL Conversion: Episode 3 #13231
Conversation
|
@swift-ci please test |
|
@swift-ci please smoke test |
|
Build failed |
|
Build failed |
Rostepher
changed the title
Rostepher
force-pushed
the
builder-dsl-episode-3
branch
from
96f4aa3 to
6fd4b80
Compare
|
@swift-ci please test |
|
@swift-ci please smoke test |
|
Build failed |
|
Build failed |
…ng if a path contains an executable.
…y of the PathType class.
…wift_build_support and return a more detailed object from ClangVersionType and SwiftVersionType.
Rostepher
force-pushed
the
builder-dsl-episode-3
branch
from
6fd4b80 to
a518ffc
Compare
|
@swift-ci please test |
|
@swift-ci please smoke test |
|
Build failed |
|
@swift-ci please test |
|
@swift-ci please smoke test |
utils/build_swift/argparse/types.py
Outdated
| if isinstance(components[0], str): | ||
| components = components[0].split('.') | ||
| elif isinstance(components[0], list): | ||
| components = tuple(components[0]) |
There was a problem hiding this comment.
Do you need to convert this to a tuple here?
There was a problem hiding this comment.
Probably not, I can remove it if you'd like.
There was a problem hiding this comment.
Yeah, removing the conversion will make it a bit clearer.
There was a problem hiding this comment.
You can convert it to isinstance(components[0], (list, tuple)).
There was a problem hiding this comment.
Huh, had no idea you could pass a tuple to isinstance. Today I learned!
| @@ -32,6 +34,44 @@ | |||
|
|
|||
| # ----------------------------------------------------------------------------- | |||
|
|
|||
| class CompilerVersion(object): | |||
There was a problem hiding this comment.
Do we already have anything like this?
There was a problem hiding this comment.
We do have an equivalent (albeit less type-safe) class in utils/swift_build_support/swift_build_support/arguments.py, however one of my main goals with the build_swift module is to completely remove reliance on that package since it's quite messy. Rather I'm opting here to implement a well-tested version analogous to the older one.
There was a problem hiding this comment.
We should add a comment to the old one indicating that it's deprecated then and link to the new one.
utils/build_swift/argparse/types.py
Outdated
| @@ -107,6 +173,12 @@ def __init__(self): | |||
| ClangVersionType.VERSION_REGEX, | |||
| ClangVersionType.ERROR_MESSAGE) | |||
|
|
|||
| def __call__(self, value): | |||
| matches = super(ClangVersionType, self).__call__(value) | |||
| components = filter(None, matches.group(1, 2, 3, 5)) | |||
There was a problem hiding this comment.
Is the filter here necessary?
There was a problem hiding this comment.
This filters out 0 which is a incorrect.
|
Build failed |
|
@swift-ci please test |
|
Build failed |
|
Build failed |
|
@swift-ci please test |
|
Build failed |
|
Build failed |
AnthonyLatsis
added
build-script
This PR is the third and final part of #13117 and similarly doesn't contain any real functional changes. I did have to add support for asserting a path contains an executable in both the
PathTypeandStorePathActionclasses and I've added associated tests. I've also converted the top-level arguments to the new builder DSL, which means that all the argument definitions are now in the builder DSL. All the unit-tests still pass which makes me confident that this PR is low risk.rdar://34336890