[build-script] Eliminate swift build support subprocess functions #2836
Conversation
…ing. - This also uses the option in update-checkout to restores the previous, less verbose, output.
- This eliminates the last client of `SwiftBuildSupport.check_call`, which is now replaced by `swift_build_support.shell.call`. - Part of SR-237.
- This is an analog to `call`, which returns the captured output of the command.
- This eliminates the last uses of `SwiftBuildSupport.check_output`.
|
@swift-ci please Python lint |
|
/cc @gribozavr @swift-ci please test |
| @@ -80,40 +88,74 @@ def call(command, stderr=None, env=None, dry_run=None): | |||
| "': " + e.strerror) | |||
|
|
|||
|
|
|||
| def capture(command, stderr=None, env=None, dry_run=None, print_command=True): | |||
There was a problem hiding this comment.
For this method, print_command=False is relevant IMO.
BTW, Could you change print_command to echo or trace? it's easier to type.
There was a problem hiding this comment.
For this method, print_command=False is relevant IMO.
Did you mean irrelevant? Or that the default should be different?
BTW, Could you change print_command to echo or trace? it's easier to type.
I agree, I would prefer a shorter name too but I was preserving the existing name carried over from SwiftBuildSupport. If @gribozavr has no objections I will switch to echo.
There was a problem hiding this comment.
Sorry, the default for print_command should be False only for capture method.
I know, it's inconsistent with other methods,
but we rarely want to "trace" side-effect less query command.
There was a problem hiding this comment.
I agree that False makes more sense as a default, but didn't want the methods to be inconsistent. There is also an argument to be made that we generally shouldn't usually be shelling out to query things, and if we are it might be just as useful for logging purposes to see what commands contributed to the build script behavior.
As the module stands I think keeping the methods consistent at the cost of slightly more verbose clients is the right tradeoff.
There was a problem hiding this comment.
+1 for echo. (@echo off anyone?)
And I also agree that even though capture would not usually be echoed, consistency is important.
|
LGTM. Please merge when you are ready. |
|
Done, will rename the parameter in a follow on. |
What's in this pull request?
This PR eliminates the subprocess-related functions in SwiftBuildSupport in favor of using the newer
swift_build_support.shellmodule. It adds aprint_commandoption to the functions in the shell module (to match the behavior of existing code), adds aswift_build_support.shell.capturefunction which is analogous tosubprocess.check_output, and migrates clients to them.Before merging this pull request to apple/swift repository:
Triggering Swift CI
The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:
Smoke Testing
Validation Testing
Lint Testing
Note: Only members of the Apple organization can trigger swift-ci.