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

Implement SE-0219 Package Manager Dependency Mirroring #1776

Merged
merged 1 commit into from
Oct 5, 2018

Conversation

aciidgh
Copy link
Contributor

@aciidgh aciidgh commented Sep 10, 2018

rdar://problem/42511642 [SR-8328]: Implement SE-0219 Package Manager Dependency Mirroring
https://bugs.swift.org/browse/SR-8328

@aciidgh aciidgh added the WIP Work in progress label Sep 10, 2018
@aciidgh aciidgh force-pushed the mirroring branch 8 times, most recently from 5fd3024 to 9192adc Compare September 14, 2018 20:56
@aciidgh aciidgh force-pushed the mirroring branch 16 times, most recently from c943fd6 to 3dd4883 Compare October 5, 2018 01:31
<rdar://problem/42511642> [SR-8328]: Implement SE-0219 Package Manager Dependency Mirroring
https://bugs.swift.org/browse/SR-8328
@aciidgh aciidgh changed the title [WIP] Implement SE-0219 Package Manager Dependency Mirroring Implement SE-0219 Package Manager Dependency Mirroring Oct 5, 2018
@aciidgh
Copy link
Contributor Author

aciidgh commented Oct 5, 2018

@swift-ci smoke test

@aciidgh aciidgh removed the WIP Work in progress label Oct 5, 2018
@aciidgh
Copy link
Contributor Author

aciidgh commented Oct 5, 2018

@swift-ci test with toolchain

@aciidgh
Copy link
Contributor Author

aciidgh commented Oct 5, 2018

@swift-ci smoke test macOS

@aciidgh
Copy link
Contributor Author

aciidgh commented Oct 5, 2018

@swift-ci smoke test OS X

@aciidgh aciidgh merged commit 9d7278c into swiftlang:master Oct 5, 2018
@aciidgh aciidgh deleted the mirroring branch October 5, 2018 02:40
return mirrors[url]?.mirror
}

/// Returns the tr

Choose a reason for hiding this comment

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

Im not sure what the tr stands for.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, this is a typo. I think I wanted to write something like:

/// Returns the mirrored url if it exists, otherwise the original url. 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed here: 00e8771


// Test deletion.
try execute(["config", "unset-mirror", "--package-url", "https://github.com/foo/bar"], packagePath: packageRoot)
try execute(["config", "unset-mirror", "--mirror-url", "[email protected]:foo/swift-package-manager.git"], packagePath: packageRoot)
Copy link

@kandelvijaya kandelvijaya Oct 14, 2018

Choose a reason for hiding this comment

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

The proposal mentions to implement unset-mirror --all but it's not yet implemented.

Proposal

https://github.com/apple/swift-evolution/blob/master/proposals/0219-package-manager-dependency-mirroring.md#dependency-mirroring

Usacase

screenshot 2018-10-14 at 07 56 39

what next?

This can be done in two ways at least from my naive eye:

  1. use the options parses as all of get-mirror and set-mirror or unset-mirror is done. Internally ask the json from file and remove each one.
  2. Parse and then remove the entire json object from the file at once. .swiftpm/config

Any thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given that we currently only store the mirror information in the config file, I don't see a need to implement the --all flag. The user can just delete config file by hand for now. We can add the flag in the future when we start adding more stuff in the config file.

@kandelvijaya
Copy link

Maybe this is too late, anyways I want to point to a scenario.

When the user attempts to set a mirror-url for given package url like such:

$ spm package config set-mirror --package-url pkg1 --mirror-url pkg2  (1)
$ spm package config get-mirror --package-url pkg1
// pkg2 

$ spm package config set-mirror --package-url pkg2 --mirror-url pkg3 (2)
/// pkg1 --mirror--> pkg2
/// pkg2 --mirror--> pkg3

1 we dont verify if the provided package-url is present in the list of dependencies user specified in Package.swift. Thereby it kind of works like setting values without much validation. I'm not sure which is better but personally (atm) I think we can verify if the package was specified.

2 This is partly a side effect of not having check as described in point 1. The main question is A -> B and B -> C. Is package C mirroring A then?

These are my opinion. Let me know what you think.

@aciidgh
Copy link
Contributor Author

aciidgh commented Oct 15, 2018

It is indeed possible to misuse the mirroring feature which could lead to weird behaviors. However, we think that the majority of users will use the feature as intended. It would be nice if we could do some sort of validation but there are several complications in doing so.

Validation of mirrored repository:
We thought about possibly verifying that the mirror is indeed a mirror of the same package and not something else. But one of the use-case of mirrors is to get access in an env. where you don't have access to the original url.

Checking the presence of dependency:
This has several issues. The package graph might have not been resolved yet. It is completely valid to have mirror entries of dependencies that are not in your graph yet. In that case, we would end up showing an error or warning, which doesn't seem correct.

We can always make changes to a feature by the swift evolution process. Let's see the response from users and we can figure out how we can improve it if they do run into weird things.

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.

2 participants