Skip to content

[bootstrap] Add swift-argument-parser as a bootstrapped swift-driver dependency #2739

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

Merged
merged 1 commit into from
Sep 9, 2020

Conversation

owenv
Copy link
Contributor

@owenv owenv commented May 11, 2020

Only the last commit is new, the rest is from #2736

Copy link
Member

@DougGregor DougGregor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after a rebase

@owenv owenv force-pushed the driver-add-argparse branch from 9060ebd to 0e86eba Compare May 13, 2020 16:58
@owenv owenv marked this pull request as draft May 13, 2020 17:05
@owenv
Copy link
Contributor Author

owenv commented May 13, 2020

Marking this as a draft until I have a chance to make the update-checkout changes

@@ -20,6 +20,7 @@ target_link_libraries(Build PUBLIC
SPMBuildCore
CYaml
Yams
ArgumentParser
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't strictly necessary yet, but I think it'll allow incremental adoption of ArgumentParser in the driver library

@owenv
Copy link
Contributor Author

owenv commented May 14, 2020

@owenv
Copy link
Contributor Author

owenv commented May 19, 2020

There might be some remaining build issues on Linux to resolve here, but let's see how it works with an updated ArgumentParser

swiftlang/swift-driver#92
@swift-ci please smoke test

@owenv
Copy link
Contributor Author

owenv commented May 19, 2020

swiftlang/swift-driver#92
@swift-ci please smoke test

2 similar comments
@owenv
Copy link
Contributor Author

owenv commented May 20, 2020

swiftlang/swift-driver#92
@swift-ci please smoke test

@owenv
Copy link
Contributor Author

owenv commented May 26, 2020

swiftlang/swift-driver#92
@swift-ci please smoke test

@DougGregor
Copy link
Member

FYI, also learned about Docker as another place to update: #2767

@tomerd
Copy link
Contributor

tomerd commented Aug 17, 2020

@DougGregor this seems to be still draft. let us know when you need a review

@DougGregor
Copy link
Member

We should go ahead and do this when it's ready. Are you stuck on something, @owenv ?

@owenv
Copy link
Contributor Author

owenv commented Aug 19, 2020

@DougGregor I think we'll want to coordinate this with @natecook1000's changes in #2653 so that CI & update-checkout can be configured to pull a single version of ArgumentParser for use in both swift-driver and SPM. I'll try and test this with swift-argument-parser 0.3 sometime in the next few days

@DougGregor
Copy link
Member

@DougGregor I think we'll want to coordinate this with @natecook1000's changes in #2653 so that CI & update-checkout can be configured to pull a single version of ArgumentParser for use in both swift-driver and SPM. I'll try and test this with swift-argument-parser 0.3 sometime in the next few days

Yeah, this absolutely makes sense.

@abertelrud
Copy link
Contributor

abertelrud commented Aug 22, 2020

#2653 is almost ready to be merged. Independently, those diffs also involved making similar changes to the bootstrap script as the diffs in this PR (I didn't realize at the time that this PR already had those changes). We can make those changes use the same variable names in the bootstrap script as this diff uses, so that when rebased, this will be a fairly clean addition to those diffs (essentially just making the Build module link against Argument parser). Does that make sense as a way to proceed?

@owenv
Copy link
Contributor Author

owenv commented Aug 22, 2020

@abertelrud That plan sounds good to me!

natecook1000 added a commit to natecook1000/swift-package-manager that referenced this pull request Aug 22, 2020
@owenv owenv force-pushed the driver-add-argparse branch from 3d44a96 to 6742650 Compare August 22, 2020 18:04
@owenv owenv marked this pull request as ready for review August 22, 2020 18:05
@owenv
Copy link
Contributor Author

owenv commented Aug 22, 2020

This is now up-to-date with swiftlang/swift-driver#92, but we'll still wait for #2653 to land before merging

@owenv owenv force-pushed the driver-add-argparse branch from 6742650 to 58dcafb Compare August 28, 2020 03:51
Don't build ArgumentParser example projects
@owenv owenv force-pushed the driver-add-argparse branch from 58dcafb to 047b349 Compare August 28, 2020 03:53
@owenv
Copy link
Contributor Author

owenv commented Aug 28, 2020

swiftlang/swift-driver#92
@swift-ci smoke test

1 similar comment
@owenv
Copy link
Contributor Author

owenv commented Aug 28, 2020

swiftlang/swift-driver#92
@swift-ci smoke test

@owenv
Copy link
Contributor Author

owenv commented Aug 28, 2020

@abertelrud
Copy link
Contributor

abertelrud commented Sep 4, 2020

@owenv Please merge when ready (or I can do it, if it can go in at any time — are you coordinating with a PR in swift-driver as well?).

@owenv
Copy link
Contributor Author

owenv commented Sep 5, 2020

@abertelrud thanks for the reminder! I just realized I forgot to request a review on the corresponding swift-driver PR (swiftlang/swift-driver#92). Once that's approved, I might need help with the cross-repository merge (I don't have write access to this repository)

@abertelrud
Copy link
Contributor

@owenv No problem, just let me know when this is ready to merge. Thanks!

@owenv
Copy link
Contributor Author

owenv commented Sep 9, 2020

@abertelrud it looks like everything is approved so this should be ready to merge now. Thanks for your help with this!

@DougGregor
Copy link
Member

Let's do it!

@DougGregor DougGregor merged commit d1c1c3a into swiftlang:master Sep 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants