Adopt the ArgumentParser library for the command-line tools #2653
Conversation
Started by converting SwiftOptions, SwiftTool, and SwiftRunTool.
This depends on adding --version support to ArgumentParser
|
@swift-ci Please smoke test |
1 similar comment
|
@swift-ci Please smoke test |
natecook1000
force-pushed
the
argumentparser
branch
from
45d1888 to
dbacb35
Compare
|
@swift-ci please smoke test self hosted |
|
@swift-ci Please smoke test self hosted |
The SwiftTool initializer performs some global configuration steps, so it isn't really safe to require that individual commands create an instance, even when they don't need one. This moves the customization point to a run(_ swiftTool: SwiftTool) method.
natecook1000
force-pushed
the
argumentparser
branch
from
fa7489e to
eb19d3a
Compare
|
@swift-ci Please smoke test self hosted |
|
@swift-ci Please smoke test self hosted |
|
@swift-ci Please smoke test self hosted |
# Conflicts: # Sources/Commands/Options.swift # Sources/Commands/SwiftBuildTool.swift # Sources/Commands/SwiftPackageTool.swift # Sources/Commands/SwiftRunTool.swift # Sources/Commands/SwiftTestTool.swift # Sources/Commands/SwiftTool.swift
Got it. For the smoke test it must be using the version that's checked out next to swiftpm rather than the dependency in the package. Although I wonder what fallout switching to 0.3.0 across the board would have on other clients... |
|
I just found your existing PR swiftlang/swift-source-compat-suite#443 that would move it to 0.2.0. Were there concerns about merging that one at the time? (there's nothing in the comments) |
|
Not that I remember, I just lost track of it at the time. @shahmishal does need to be cc'ed on any changes in this area though because I think there's some Jenkins configuration required to change checkout tags for security reasons.
This might be a problem in the future, but I don't think there are any other clients right now. The checkout was originally setup alongside the swift-driver CMake build but there were some issues preventing adoption for awhile. |
|
Sounds great. Would you be willing to update your existing PR (which Mishal already approved once) to 0.3.0 and then we can ask for his re-approval with the updated version? I could open a new PR, but since you already have one open for essentially the same thing, we might as well reuse it. We'll block merging of this PR on the merging of that one. |
|
I messed up a force push, so I had to re-create the PRs, but they're available here: |
|
Thanks, very much appreciated! |
|
At this point this is just waiting for the CI to be configured to check out 0.3.0 of ArgumentParser (those are the PRs that @owenv mentioned; they just need to be approved and merged, then we can merge this). |
|
@swift-ci Please smoke test |
|
Now that the other two PRs are merged, we just need these tests to pass and then can merge this. 🤞 |
|
The failure is unrelated ( |
|
Now that #2883 is merged, we can try this CI again. |
|
@swift-ci please smoke test |
|
The Linux failure seems to involve an error in SourceKit-LSP's |
|
It does look related: 15:13:49 + /home/buildnode/jenkins/workspace/swift-package-manager-Linux-smoke-test/branch-master/sourcekit-lsp/Utilities/build-script-helper.py build --package-path /home/buildnode/jenkins/workspace/swift-package-manager-Linux-smoke-test/branch-master/sourcekit-lsp --build-path /home/buildnode/jenkins/workspace/swift-package-manager-Linux-smoke-test/branch-master/build/buildbot_incremental/sourcekitlsp-linux-x86_64 --configuration release --toolchain /home/buildnode/jenkins/workspace/swift-package-manager-Linux-smoke-test/branch-master/build/buildbot_incremental/toolchain-linux-x86_64/usr --ninja-bin /home/buildnode/jenkins/workspace/swift-package-manager-Linux-smoke-test/branch-master/build/buildbot_incremental/ninja-build/ninja --verbose So this might be a difference in how this flag is being parsed. |
|
I don't see any problems with how the |
|
I can actually reproduce this using the exact line that SourceKit-LSP is passing: with the changes in this PR, but it doesn't reproduce without them. I'll see if I can figure out what's causing it to think it's seeing a |
|
Looks like the C++ compiler flag option had an incorrect name. Since |
|
@swift-ci Please smoke test |
|
Fantastic; thank you! I guess this wasn't seen on macOS because the script only passes that flag on Linux? Not sure what happened there. I guess I also wonder whether there are other combinations of flags that might get passed and that might be misinterpreted as combined single-file flags, resulting in confusing error messages. Improving handling of incorrectly specified parameters should not prevent us from merging, of course, but I am wondering whether we should disable combining of short-from flags for SwiftPM. |
|
@natecook1000, assuming this passes, are you fine with merging it? I think all the other issues have been taken care of (downstream CI etc). |
…g#2653) This transitions the Commands tools to use ArgumentParser instead of the TSC parser. The biggest change is that the tools are no longer subclasses of SwiftTool. Instead, each tool conforms to the SwiftCommand protocol, which requires the shared options and creates a SwiftTool instance after parsing.
…wiftlang#2653)" This reverts commit b30b5e2.
This transitions the different SPM command-line tools to use the
ArgumentParserlibrary instead of the TSC parser. Among the changes is that the tools are no longer subclasses ofSwiftTool. Instead, each tool creates aSwiftToolinstance to use when necessary. It may be useful to follow up with a refactoring to either move some of the functionality that's in the tools intoSwiftToolor to otherwise separate the CLI from the actual functions being performed.Still to do:
ArgumentParser