Allow for .package(url: , branch:) syntax#3292
Conversation
Allows for a more natural way to use a branch requirement when compared to .package(url: , .branch()) That option is still available though
miggs597
requested review from
abertelrud,
friedbunny,
neonichu and
tomerd
as code owners
abertelrud
left a comment
abertelrud
left a comment
There was a problem hiding this comment.
Thanks for this change! The lack of this convenience initializer has always been annoying to me.
The implementation itself looks good apart from the availability annotation, which is noted in the code. In addition to unit tests, it would also be a good idea to update the change log and to update the PackageDescription.md file at the same time.
| /// - name: The name of the package, or nil to deduce it from the URL. | ||
| /// - url: The valid Git URL of the package. | ||
| /// - branch: A dependency requirement. See static methods on `Package.Dependency.Requirement` for available options. | ||
| @available(_PackageDescription, introduced: 5.3) |
There was a problem hiding this comment.
This should be 999.0 until the version number of the next release is announced.
There was a problem hiding this comment.
Ah okay, I wasn't sure so I just did the current version, I'll take care of that and the other parts in the comment
|
@swift-ci please smoke test |
|
@swift-ci please smoke test |
| * [#2937] | ||
| * Improvements | ||
|
|
||
| Adding a dependency requirement can now be done with the convenience initializer .package(url: , branch:) |
There was a problem hiding this comment.
Nit: I suggest backticks around the code so it gets styled differently.
| dependencies: [ | ||
| // Dependencies declare other packages that this package depends on. | ||
| // .package(url: /* package url */, from: "1.0.0"), | ||
| .package(url: "../Foo", branch: "master") |
There was a problem hiding this comment.
Should be using main. Or is that baked into the test fixture? (which we should eventually fix too — not necessarily as part of this PR).
There was a problem hiding this comment.
That's baked into the fixture, it's just that the version of git the test uses hasn't been updated to use main instead of master
There was a problem hiding this comment.
Hmm... I wonder if that means that this test might unintentionally depend on the version of git that's installed. It might make sense to do what some of the other tests do (e.g. testMirrors) which would be to add something like:
Process.checkNonZeroExit(args: "git", "-C", path.pathString, "init")
Process.checkNonZeroExit(args: "git", "-C", path.pathString, "checkout", "-b", "mybranch")
and then have the dependency use that. That would make it independent of what the default branch was named.
There was a problem hiding this comment.
I think we have two kinds of test fixtures: those that have a .zip file containing a .git with prebaked repository contents (branch names, tags, etc), and plain directories, which the fixture() function sets up to have a newly initialized repository. For the latter kind, the name of the initial repository depends on the version of Git installed.
Maybe the right thing to do is to modify the initGitRepo() to check whether there's a main in the newly created repository and if not to do a:
Process.checkNonZeroExit(args: "git", "-C", path.pathString, "branch", "-m", "main")
so that all our unit tests can start out in a known state regardless of the version of Git installed on the machine running the test.
There was a problem hiding this comment.
Then your new unit test wouldn't have to be modified (though some others might have to be adjusted to account for it). But it seems suboptimal that these tests will start failing once versions of Git that default to main start getting deployed more widely.
There was a problem hiding this comment.
What if we add
try systemQuietly([Git.tool, "-C", dir.pathString, "checkout", "-b", "main"])
to initGitRepo() ensuring there's always a main branch?
There was a problem hiding this comment.
I think that would fail if there's already a main, so I suggest we do:
branch -m main
I tested that that is a no-op if the branch is already named main.
There was a problem hiding this comment.
But in general I agree: could you modify initGitRepo() to do the branch -m main? That would make all the tests a lot more robust. Thanks!
There was a problem hiding this comment.
Yep, just got it working with branch -m main!
|
@swift-ci please smoke test |
tomerd
added
the
ready
friedbunny
left a comment
friedbunny
left a comment
There was a problem hiding this comment.
This is quite a nice change, thanks.
Refactoring instances of 'master' into 'main'
abertelrud
left a comment
abertelrud
left a comment
There was a problem hiding this comment.
Looks good to me! Thanks!
|
@swift-ci please smoke test |
This might be one for @tomerd, https://ci.swift.org/job/swift-package-manager-Linux-smoke-test/2982/ |
|
@swift-ci please smoke test linux |
5 participants
Improve quality of life when using a branch for a package dependency requirement.
It feels more natural to use
.package(url:, branch:)as opposed to.package(url:,.branch())rdar://55857600