Skip to content
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

Argument Parser and Swift 5 #1608

Closed
wants to merge 40 commits into from

Conversation

Wevah
Copy link
Contributor

@Wevah Wevah commented Jun 11, 2020

Updates various Swift tools to use Swift 5 and the Argument Parser package.

Since the tools are currently distributed without an embedded Swift runtime, users on OSs before 10.14.4 would have already needed to install the shared Swift libs.

Care is being taken to preserve the old semantics and help text as much as possible (with a few stylistic changes for consistency with Argument Parser, etc.).

(#1583)

@Wevah
Copy link
Contributor Author

Wevah commented Jun 11, 2020

This unfortunately probably won't compile under Xcode 10, but the work will be done if/when the tools no longer need to build under Xcode 10.

Edit: Apparently Travis needs some extra work to make it deal with SPM pulls.

@Wevah Wevah changed the title Argument parser and swift 5 Argument Parser and Swift 5 Jun 11, 2020
@kornelski
Copy link
Member

Thank you

@Wevah
Copy link
Contributor Author

Wevah commented Jun 13, 2020

Re: Travis: It's complaining about SSH fingerprints, which is weird, since swift-argument-parser is on GitHub. 🤔

@Wevah
Copy link
Contributor Author

Wevah commented Jun 13, 2020

Apparently SPM-in-Xcode needs Xcode 11 (whoops), even though it should be included stand-alone in Xcode 10?

A workaround for all of this could be to check out swift-argument-parser as a regular git submodule.

@Wevah
Copy link
Contributor Author

Wevah commented Jun 13, 2020

If building on Xcode 10.3 should still be supported, I may need to revert to Swift 4.2 (though it looks like SAP should still build), but I won't know until I can fix these CI issues. 😖

@Wevah
Copy link
Contributor Author

Wevah commented Jun 16, 2020

Switching to HTTPS for checkout of the package fixed the error there, but now some tests are failing that are completely unrelated to anything I've touched. 🤔

Possibly because of the move to Swift 5. Will investigate.

Edit: Seems like a fluke; they pass locally (?).

@kornelski
Copy link
Member

I've restarted CI, but it consistently fails on:

Missing package product 'ArgumentParser', please fix package resolution errors before building (in target 'generate_keys')

@Wevah
Copy link
Contributor Author

Wevah commented Jun 18, 2020

I've restarted CI, but it consistently fails on:

Missing package product 'ArgumentParser', please fix package resolution errors before building (in target 'generate_keys')

Ah, it's in the 10.3 build. I'll have to figure something out there.

(11.5 build is working now, so that's good!)

@Wevah
Copy link
Contributor Author

Wevah commented Jun 18, 2020

The issue might be that Xcode 10 doesn't have built-in support for SPM. I'll look at other options (old-fashioned git submodule might work).

Edit: Actually, calling SPM manually pre-build on Xcode 10 might work.

@zorgiepoo
Copy link
Member

We don't use travis now and this PR should be rebased before being further reviewed. We also bumped the Xcode version to at least 11 in CI (the repo ReadMe was updated regarding the policy change for what versions should be supported further onwards). Another recent policy change we made is that changes such as this one will also need to land in 2.x branch first.

@zorgiepoo
Copy link
Member

I used the generate_keys changes here as a reference when implementing #1730 so this was quite useful to look at. I didn't write a way, like here, to use another/newly created keychain however because I wanted to still use the login keychain and didn't want to have to unlock other keychains. My rationale is if we use the login keychain to store the data, we should also be able to import to there. Maybe having another option to use a custom keychain still has its own use case too. (Also generate_keys is unfortunately pretty out of date in the master branch which important enough for me to address).

@Wevah
Copy link
Contributor Author

Wevah commented Jan 27, 2021

I haven't touched this in a while, but IIRC the export option was just to move it from the login keychain to a separate keychain (maybe that's what you said; my brain is a bit fried).

Agreed about being able to import back to the login keychain.

@zorgiepoo
Copy link
Member

I just noticed 2.x has been using Swift 5 for some while now (not sure when). Adopting Argument Parser (this is welcome) and using a separate keychain (if people still want that since you can import to login keychain now) need to be in separate PR's. Closing due to staleness.

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.

None yet

3 participants