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

Add support for Netrc for Downloader #88

Merged
merged 23 commits into from
Aug 18, 2020
Merged

Conversation

sstadelman
Copy link
Contributor

@sstadelman sstadelman commented Jul 14, 2020

Implemented handling for netrc files. Matches connection settings for binary artifact host, and returns Basic authentication header to be appended to internal URLRequest of the Downloader. Will resolve Supporting basic auth for non-git binary dependency hosts.

Netrc parser based on Carthage implementation and tests, and migrated to regex for flexibility.

Features supported

  • Parse Netrc string text: single and multi-line
  • Match machine, login, password key values
  • Ignore comments
  • Ignore account and macdef entries (relevant only to ftp remotes)
  • Accept any order of login, password, account key values
  • Match default connection settings.
  • Validates single instance of default connection setting, and that default connection setting is last entry
  • Checks for netrc file in NSHomeDirectory()/.netrc by default
  • Workspace supports custom netrcFilePath parameter

Sequence Flow (including SPM)

Screen Shot 2020-07-18 at 12 13 19 AM

In SPM, a netrc file absolute path is resolved during (3) getActiveWorkspace() call of run() procedure, from (1) optional command argument. Optional absolute path is passed to initialized Workspace(5).

If binary artifacts should be updated, then the (7) download(...) operation attempts to load the netrc machine definitions from a file at the resolved netrcFilePath(8), or at NSHomeDirectory()/.netrc if nil. The optional resulting Netrc instance is passed to the created FoundationDownloader for each artifact (11). The downloader attempts to match a credential in the list of the Netrc instance's Machine definitions for the artifact host (13). If matched, the base64-encoded <user>:<password> pair is set to the Authorization header for the respective URLRequest.

Notes

  1. The main question wrt API addition was: how to efficiently pass the netrc content to the Downloader protocol, which is lightweight by design. Though netrc is a common technique, it is not necessarily a universal solution--perhaps in the future we should also support application tokens, etc., which might similarly pollute the protocol. So, this implementation introduces the AuthorizationProviding interface, under which additional authentication techniques could in the future be added to the Workspace, and injected to the binary update process in Downloader implementations.
public protocol AuthorizationProviding {
    /// Optional `Authorization` header, likely added to `URLRequest`
    func authorization(for url: Foundation.URL) -> String?
}

Downloader is extended as follows:

/// The `Downloader` protocol abstract away the download of a file with a progress report.
public protocol Downloader {
    /**/
    /// Downloads a file and keeps the caller updated on the progress and completion.
    ///
    /// - Parameters:
    ///   - url: The `URL` to the file to download.
    ///   - destination: The `AbsolutePath` to download the file to.
    ///   - authorizationProvider: Optional provider supplying `Authorization` header to be added to `URLRequest`.
    ///   - progress: A closure to receive the download's progress as number of bytes.
    ///   - completion: A closure to be notifed of the completion of the download.
    func downloadFile(
        at url: Foundation.URL,
        to destination: AbsolutePath,
        withAuthorizationProvider authorizationProvider: AuthorizationProviding?,
        progress: @escaping Progress,
        completion: @escaping Completion
    )
}
  1. The netrc credential injection is added to FoundationDownloader. Other Downloaders should implement handling according to their requirements.
  2. Requires OSX 10.13. Will obligate swift-driver and swift-package-manager packages to raise minimum to 10.13 also. Discussed in thread.
  3. An associated PR will be necessary in swift-package-manager to support --netrc-file option. However, the default NSHomeDirectory()/.netrc location for the netrc file can be supported immediately upon merging this PR in TSC.

@sstadelman
Copy link
Contributor Author

@swift-ci test

@sstadelman sstadelman changed the title [DO NOT MERGE] Add support for Netrc for Downloader [READY FOR REVIEW] Add support for Netrc for Downloader Jul 19, 2020
@sstadelman
Copy link
Contributor Author

@swift-ci test

1 similar comment
@neonichu
Copy link
Contributor

@swift-ci test

@sstadelman
Copy link
Contributor Author

@swift-ci smoke-test

@sstadelman
Copy link
Contributor Author

Mirrored in spm: swiftlang/swift-package-manager#2833.

@neonichu
Copy link
Contributor

@swift-ci please test

@MaxDesiatov
Copy link
Contributor

@sstadelman would you be able to resolve the conflict?

@sstadelman
Copy link
Contributor Author

@MaxDesiatov conflict resolved.

@MaxDesiatov
Copy link
Contributor

@swift-ci please test

@MaxDesiatov
Copy link
Contributor

@swift-ci please smoke test

@MaxDesiatov
Copy link
Contributor

@swift-ci please test

@MaxDesiatov
Copy link
Contributor

The Linux build is failing:

error: reference to property 'downloads' in closure requires explicit 'self.' to make capture semantics explicit

@MaxDesiatov
Copy link
Contributor

@swift-ci please test

@MaxDesiatov MaxDesiatov changed the title [READY FOR REVIEW] Add support for Netrc for Downloader Add support for Netrc for Downloader Aug 11, 2020
@MaxDesiatov
Copy link
Contributor

@swift-ci please test

@MaxDesiatov
Copy link
Contributor

MaxDesiatov commented Aug 11, 2020

@sstadelman the build still fails on Linux, do you get the same error messages when you build and test on Linux with Swift 5.2 on your side? (The Linux Swift CI node uses Swift 5.2)

@sstadelman
Copy link
Contributor Author

@MaxDesiatov it's a different failure message, regarding AbsolutePath optionality. Will test on Linux with 5.2 and ping you when complete. Sorry about that.

@sstadelman
Copy link
Contributor Author

@MaxDesiatov resolved. passed on Swift:latest tag.

@MaxDesiatov
Copy link
Contributor

@swift-ci please test

1 similar comment
@MaxDesiatov
Copy link
Contributor

@swift-ci please test

@sstadelman
Copy link
Contributor Author

@swift-ci please test

Copy link
Contributor

@MaxDesiatov MaxDesiatov left a comment

Choose a reason for hiding this comment

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

Awesome, thanks 👍

@MaxDesiatov
Copy link
Contributor

@swift-ci please test

1 similar comment
@MaxDesiatov
Copy link
Contributor

@swift-ci please test

@MaxDesiatov
Copy link
Contributor

Hi @neonichu, would you mind having a look at this? Thank you!

Copy link
Contributor

@neonichu neonichu left a comment

Choose a reason for hiding this comment

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

This looks great, thanks @sstadelman for the implementation and @MaxDesiatov for helping with the review!

@neonichu neonichu merged commit ed31a0c into swiftlang:master Aug 18, 2020
@sstadelman
Copy link
Contributor Author

Thanks @neonichu and @MaxDesiatov for your assistance along the way!

@neonichu: what do you expect as the timeline for this being picked up by the standard Xcode distribution?

@MaxDesiatov
Copy link
Contributor

MaxDesiatov commented Aug 18, 2020

Xcode 12 GM won't get this unless the SwiftPM release manager accepts this same PR to the release/5.3 branch. If not, it would be included in the version of Swift following after 5.3, whatever version that may be (and it's not announced yet as far as I know)

compnerd added a commit to compnerd/swift-package-manager that referenced this pull request Aug 18, 2020
The new parameter seems to trip up the build on Windows.  This adds the
parameter in explicitly which repairs the build.
@OdNairy
Copy link

OdNairy commented Sep 16, 2020

Xcode 12 GM Seed 1 (12A7209) doesn't support .netrc auth, right?

@MaxDesiatov
Copy link
Contributor

I don't think this PR was re-submitted for the release/5.3 branch for that to happen, was it?

@sstadelman
Copy link
Contributor Author

I don't think this PR was re-submitted for the release/5.3 branch for that to happen, was it?

No, is that the process for adoption in an upcoming patch?

@MaxDesiatov
Copy link
Contributor

MaxDesiatov commented Sep 17, 2020

It doesn't guarantee that it will end up in a 5.3 patch release, but I guess it will be at least considered then if the change is not too disruptive and is well-tested. A similar situation happened to swiftlang/swift-package-manager#2905, which missed the 5.3.0 cut-off.

sstadelman pushed a commit to sstadelman/swift-tools-support-core that referenced this pull request Sep 23, 2020
Add support for Netrc for Downloader
@sstadelman
Copy link
Contributor Author

@MaxDesiatov what is the current status of this repository vis-a-vis swift-package-manager? Do all changes still need to be mirrored in both?

@MaxDesiatov
Copy link
Contributor

Yes, definitely in the release/5.3 branch, and still in main until swiftlang/swift-package-manager#2898 is merged.

xymus pushed a commit to xymus/swift-driver that referenced this pull request Nov 6, 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