[Review] SE-0150 Package Manager Support for branches


(Daniel Dunbar) #1

Hello Swift community,

The review of SE-0150 “ Package Manager Support for branches" begins now and runs through January 31, 2017. The proposal is available here:

  https://github.com/apple/swift-evolution/blob/master/proposals/0150-package-manager-branch-support.md

Reviews are an important part of the Swift evolution process. All reviews should be sent to the swift-build-dev and swift-evolution mailing lists at
  https://lists.swift.org/mailman/listinfo/swift-build-dev
  https://lists.swift.org/mailman/listinfo/swift-evolution
or, if you would like to keep your feedback private, directly to the review manager. When replying, please try to keep the proposal link at the top of the message:
  https://github.com/apple/swift-evolution/blob/master/proposals/0150-package-manager-branch-support.md

What goes into a review?

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of Swift. When writing your review, here are some questions you might want to answer in your review:

  • What is your evaluation of the proposal?
  • Is the problem being addressed significant enough to warrant a change to Swift?
  • Does this proposal fit well with the feel and direction of Swift?
  • If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
  • How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
More information about the Swift evolution process is available at

https://github.com/apple/swift-evolution/blob/master/process.md
Thank you,

- Daniel

Review Manager


(David Hart) #2

Hello Swift community,

The review of SE-0150 “ Package Manager Support for branches" begins now and runs through January 31, 2017. The proposal is available here:

https://github.com/apple/swift-evolution/blob/master/proposals/0150-package-manager-branch-support.md

Reviews are an important part of the Swift evolution process. All reviews should be sent to the swift-build-dev and swift-evolution mailing lists at
https://lists.swift.org/mailman/listinfo/swift-build-dev
https://lists.swift.org/mailman/listinfo/swift-evolution
or, if you would like to keep your feedback private, directly to the review manager. When replying, please try to keep the proposal link at the top of the message:
https://github.com/apple/swift-evolution/blob/master/proposals/0150-package-manager-branch-support.md

What goes into a review?

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of Swift. When writing your review, here are some questions you might want to answer in your review:

   • What is your evaluation of the proposal?

This is going to be immensely useful. I remember depending on many `swift3` branches during the summer on my current iOS dependency manager of choice. Also applies for bug fix branches, experimental feature branches, etc...

   • Is the problem being addressed significant enough to warrant a change to Swift?

Yes. Very significant.

   • Does this proposal fit well with the feel and direction of Swift?

Yes.

   • If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?

All other dependent managers I've used had this feature and I'v ended up using it fairly often.

   • How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

Quick reading

···

On 24 Jan 2017, at 19:18, Daniel Dunbar via swift-evolution <swift-evolution@swift.org> wrote:


#3

Hello Swift community,

The review of SE-0150 “ Package Manager Support for branches" begins now and runs through January 31, 2017. The proposal is available here:

  https://github.com/apple/swift-evolution/blob/master/proposals/0150-package-manager-branch-support.md

Reviews are an important part of the Swift evolution process. All reviews should be sent to the swift-build-dev and swift-evolution mailing lists at
  https://lists.swift.org/mailman/listinfo/swift-build-dev
  https://lists.swift.org/mailman/listinfo/swift-evolution
or, if you would like to keep your feedback private, directly to the review manager. When replying, please try to keep the proposal link at the top of the message:
  https://github.com/apple/swift-evolution/blob/master/proposals/0150-package-manager-branch-support.md

What goes into a review?

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of Swift. When writing your review, here are some questions you might want to answer in your review:

  • What is your evaluation of the proposal?

This has been a feature request of mine for quite a while now. I'm a supporter of this proposal. Also having the actual revision number in the pin file is a great way to make sure everyone useses the exact same revision (if the pin file is also versioned).

  • Is the problem being addressed significant enough to warrant a change to Swift?

Yes.

  • Does this proposal fit well with the feel and direction of Swift?

Yes.

  • If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?

I've also used pip (python), which something similar. However pip's syntax for installing from a branch is more cumbersome and different to installing the latest release; e.g. (taken from SO):

   pip install django-oscar-paypal
   pip install git+https://github.com/tangentlabs/django-oscar-paypal.git@issue/34/oscar-0.6

  • How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

Quick reading.

···

On 2017-01-24 18:18:07 +0000, Daniel Dunbar via swift-build-dev said:

More information about the Swift evolution process is available at

https://github.com/apple/swift-evolution/blob/master/process.md
Thank you,

- Daniel

Review Manager
_______________________________________________
swift-build-dev mailing list
swift-build-dev@swift.org
https://lists.swift.org/mailman/listinfo/swift-build-dev

--
-Bouke


(Martin Waitz) #4

Hello,

The review of SE-0150 “ Package Manager Support for branches" begins
now and runs through January 31, 2017. The proposal is available here:

https://github.com/apple/swift-evolution/blob/master/proposals/0150-package-manager-branch-support.md

What is your evaluation of the proposal?

-1
I think we should more clearly separate the contents of the manifest and the version pins file.
I don't like to have two competing ways to do the same thing.

The manifest file (`Packages.swift`) should contain all required dependencies while the pins file should contain specific versions.

If there the dependency has no released version yet that is fine!
But instead of hard-coding some version into the manifest, we should allow to specify dependencies without any version requirement.
The concrete branch/version specification should always be stored in the `Package.pins` file.
This way, we also don't have any problem with conflicting branch specifications.

Is the problem being addressed significant enough to warrant a change to Swift?

yes.
Bootstrapping and the ability to work with unreleased dependencies are important.

Does this proposal fit well with the feel and direction of Swift?

no.
I'd like to keep the clear separation between `Package.swift` and `Package.pins`.

···

Am 2017-01-24 19:18, schrieb Daniel Dunbar via swift-evolution:

If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?

-

How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

Quick Reading.

--
Martin


(David Sweeris) #5

Hello Swift community,

The review of SE-0150 “ Package Manager Support for branches" begins now and runs through January 31, 2017. The proposal is available here:

https://github.com/apple/swift-evolution/blob/master/proposals/0150-package-manager-branch-support.md

Reviews are an important part of the Swift evolution process. All reviews should be sent to the swift-build-dev and swift-evolution mailing lists at
https://lists.swift.org/mailman/listinfo/swift-build-dev
https://lists.swift.org/mailman/listinfo/swift-evolution
or, if you would like to keep your feedback private, directly to the review manager. When replying, please try to keep the proposal link at the top of the message:
https://github.com/apple/swift-evolution/blob/master/proposals/0150-package-manager-branch-support.md

What goes into a review?

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of Swift. When writing your review, here are some questions you might want to answer in your review:

  • What is your evaluation of the proposal?

+1
I would even go a bit further and suggest that "Package(url: String, branch: String)” should have “master” be the default value for `branch`, to make it simpler for small developer teams who don’t necessarily have all the collaboration issues of large projects.

  • Is the problem being addressed significant enough to warrant a change to Swift?

IMHO, yes. Particularly since Xcode doesn’t support git tags (that I can find, anyway)

  • Does this proposal fit well with the feel and direction of Swift?

IMOH, yes. This proposal will make development with much easier for programmers whose work-flow is branch-based rather than tag-based.

  • How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

An embarrassingly long time trying to figure out why a package wasn’t seeing changes I’d committed to one of its dependencies... Somewhere along the line I must’ve accidentally created a branch called “1.0.0”, which lead to me conflating the 1.0.0 tag with the 1.0.0 branch, which has caused me no end of head scratching.

- Dave Sweeris

···

On Jan 24, 2017, at 10:18 AM, Daniel Dunbar <daniel_dunbar@apple.com> wrote:


(Boris Buegling) #6

Hi Martin,

Hello,

The review of SE-0150 “ Package Manager Support for branches" begins
now and runs through January 31, 2017. The proposal is available here:
https://github.com/apple/swift-evolution/blob/master/proposals/0150-package-manager-branch-support.md

What is your evaluation of the proposal?

-1
I think we should more clearly separate the contents of the manifest and the version pins file.
I don't like to have two competing ways to do the same thing.

The manifest file (`Packages.swift`) should contain all required dependencies while the pins file should contain specific versions.

If there the dependency has no released version yet that is fine!
But instead of hard-coding some version into the manifest, we should allow to specify dependencies without any version requirement.
The concrete branch/version specification should always be stored in the `Package.pins` file.
This way, we also don't have any problem with conflicting branch specifications.

While this is true for most packages, some have a development model where they will always depend on a branch and never on a version. In that case, it is more semantically correct to have the branch definition in the manifest, otherwise the package would not be able to build without a pins file.

This is especially true for the example from the proposal:

A -> (B:master, C:master) B -> D:branch1 C -> D:branch2

Since the pins file of packages B and C would not be considered when building A, you would need to lift the pinning that is inherent to those packages up to the pins file of A.

Additionally, I think the proposed errors for invalid dependencies should ensure that this will not become an issue in practice.

Cheers,
Boris

···

On 25 Jan 2017, at 10:40, Martin Waitz via swift-evolution <swift-evolution@swift.org> wrote:
Am 2017-01-24 19:18, schrieb Daniel Dunbar via swift-evolution:

Is the problem being addressed significant enough to warrant a change to Swift?

yes.
Bootstrapping and the ability to work with unreleased dependencies are important.

Does this proposal fit well with the feel and direction of Swift?

no.
I'd like to keep the clear separation between `Package.swift` and `Package.pins`.

If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?

-

How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

Quick Reading.

--
Martin
_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution


(TJ Usiyan) #7

• What is your evaluation of the proposal?
+ 1
        • Is the problem being addressed significant enough to warrant a
change to Swift?
Yes
        • Does this proposal fit well with the feel and direction of Swift?
Yes
        • If you have used other languages or libraries with a similar
feature, how do you feel that this proposal compares to those?
Yes. It's about the same.
        • How much effort did you put into your review? A glance, a quick
reading, or an in-depth study?
More information about the Swift evolution process is available at
A quick read.

···

On Mon, Jan 30, 2017 at 3:41 PM, David Sweeris via swift-evolution < swift-evolution@swift.org> wrote:

> On Jan 24, 2017, at 10:18 AM, Daniel Dunbar <daniel_dunbar@apple.com> > wrote:
>
> Hello Swift community,
>
> The review of SE-0150 “ Package Manager Support for branches" begins now
and runs through January 31, 2017. The proposal is available here:
>
> https://github.com/apple/swift-evolution/blob/master/
proposals/0150-package-manager-branch-support.md
>
> Reviews are an important part of the Swift evolution process. All
reviews should be sent to the swift-build-dev and swift-evolution mailing
lists at
> https://lists.swift.org/mailman/listinfo/swift-build-dev
> https://lists.swift.org/mailman/listinfo/swift-evolution
> or, if you would like to keep your feedback private, directly to the
review manager. When replying, please try to keep the proposal link at the
top of the message:
> https://github.com/apple/swift-evolution/blob/master/
proposals/0150-package-manager-branch-support.md
>
> What goes into a review?
>
> The goal of the review process is to improve the proposal under review
through constructive criticism and, eventually, determine the direction of
Swift. When writing your review, here are some questions you might want to
answer in your review:
>
> • What is your evaluation of the proposal?
+1
I would even go a bit further and suggest that "Package(url: String,
branch: String)” should have “master” be the default value for `branch`, to
make it simpler for small developer teams who don’t necessarily have all
the collaboration issues of large projects.

> • Is the problem being addressed significant enough to warrant a
change to Swift?
IMHO, yes. Particularly since Xcode doesn’t support git tags (that I can
find, anyway)

> • Does this proposal fit well with the feel and direction of Swift?
IMOH, yes. This proposal will make development with much easier for
programmers whose work-flow is branch-based rather than tag-based.

> • How much effort did you put into your review? A glance, a quick
reading, or an in-depth study?
An embarrassingly long time trying to figure out why a package wasn’t
seeing changes I’d committed to one of its dependencies... Somewhere along
the line I must’ve accidentally created a branch called “1.0.0”, which lead
to me conflating the 1.0.0 tag with the 1.0.0 branch, which has caused me
no end of head scratching.

- Dave Sweeris
_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution


(Rick Ballard) #8

Hello Swift community,

The review of SE-0150 “ Package Manager Support for branches" begins now and runs through January 31, 2017. The proposal is available here:

https://github.com/apple/swift-evolution/blob/master/proposals/0150-package-manager-branch-support.md

Reviews are an important part of the Swift evolution process. All reviews should be sent to the swift-build-dev and swift-evolution mailing lists at
https://lists.swift.org/mailman/listinfo/swift-build-dev
https://lists.swift.org/mailman/listinfo/swift-evolution
or, if you would like to keep your feedback private, directly to the review manager. When replying, please try to keep the proposal link at the top of the message:
https://github.com/apple/swift-evolution/blob/master/proposals/0150-package-manager-branch-support.md

What goes into a review?

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of Swift. When writing your review, here are some questions you might want to answer in your review:

  • What is your evaluation of the proposal?

+1
I would even go a bit further and suggest that "Package(url: String, branch: String)” should have “master” be the default value for `branch`, to make it simpler for small developer teams who don’t necessarily have all the collaboration issues of large projects.

This would make it easier to bring up your first set of packages without typing in as many parameters. But I have a couple concerns:

– A user who forgets to supply a second parameter could wind up accidentally depending on master when they didn't intend to. The normal case is to depend on a versioned tag, and we shouldn't make it too easy to mistakenly do otherwise.

– This would make it less obvious to someone reading a Package.swift manifest that a one of the dependencies is untagged. That's a potentially dangerous situation if you're not expecting it, so it should be visible, not implicit.

I think we should err on the side of making this explicit, since it is important, even if it means a little bit of extra typing when you're bringing up your first packages.

  - Rick

···

On Jan 30, 2017, at 12:41 PM, David Sweeris via swift-evolution <swift-evolution@swift.org> wrote:

On Jan 24, 2017, at 10:18 AM, Daniel Dunbar <daniel_dunbar@apple.com> wrote:

  • Is the problem being addressed significant enough to warrant a change to Swift?

IMHO, yes. Particularly since Xcode doesn’t support git tags (that I can find, anyway)

  • Does this proposal fit well with the feel and direction of Swift?

IMOH, yes. This proposal will make development with much easier for programmers whose work-flow is branch-based rather than tag-based.

  • How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

An embarrassingly long time trying to figure out why a package wasn’t seeing changes I’d committed to one of its dependencies... Somewhere along the line I must’ve accidentally created a branch called “1.0.0”, which lead to me conflating the 1.0.0 tag with the 1.0.0 branch, which has caused me no end of head scratching.

- Dave Sweeris
_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution


(Martin Waitz) #9

Hello Boris,

···

Am 25.01.2017 um 19:10 schrieb Boris Buegling <bbuegling@apple.com>:

But instead of hard-coding some version into the manifest, we should allow to specify dependencies without any version requirement.
The concrete branch/version specification should always be stored in the `Package.pins` file.
This way, we also don't have any problem with conflicting branch specifications.

While this is true for most packages, some have a development model where they will always depend on a branch and never on a version. In that case, it is more semantically correct to have the branch definition in the manifest, otherwise the package would not be able to build without a pins file.

This is especially true for the example from the proposal:

A -> (B:master, C:master) B -> D:branch1 C -> D:branch2

Since the pins file of packages B and C would not be considered when building A, you would need to lift the pinning that is inherent to those packages up to the pins file of A.

OK, you are right, branches can be helpful to have in the manifest.
But I’m still confident that we must not put explicit version information into it.
They belongs into the `Package.pins` file.
That is enough to get reproducible builds.


Martin


(Rick Ballard) #10

Hi Martin,

By "explicit version information", do you mean that you shouldn't put a git revision hash in the manifest – only branches and version tags should be acceptable?

I'd agree that the revision-based initializer is a marginal feature; normally your package should depend on a version range or is tracking a branch. That said, we can imagine that you might wind up needing to peg your dependency to a specific revision that isn't a version tag, and not track a moving branch, so this seemed like a fairly harmless feature to add for that case. What is your objection about supporting that?

The decision about whether to put this information in your pins or in your manifest should be driven by whether it's something your package requires, or is just a workflow choice. If you just want to temporarily stick to a specific revision of your dependency for workflow reasons, pinning is the right way to do that. If, however, your package currently requires that revision, and isn't going to work properly without it, then the manifest is the right way to express that. You'd want that specification to stick even if someone did `swift package update --repin` to get newer pinned revisions.

  - Rick

···

On Jan 25, 2017, at 11:06 PM, Martin Waitz via swift-evolution <swift-evolution@swift.org> wrote:

Hello Boris,

Am 25.01.2017 um 19:10 schrieb Boris Buegling <bbuegling@apple.com <mailto:bbuegling@apple.com>>:

But instead of hard-coding some version into the manifest, we should allow to specify dependencies without any version requirement.
The concrete branch/version specification should always be stored in the `Package.pins` file.
This way, we also don't have any problem with conflicting branch specifications.

While this is true for most packages, some have a development model where they will always depend on a branch and never on a version. In that case, it is more semantically correct to have the branch definition in the manifest, otherwise the package would not be able to build without a pins file.

This is especially true for the example from the proposal:

A -> (B:master, C:master) B -> D:branch1 C -> D:branch2

Since the pins file of packages B and C would not be considered when building A, you would need to lift the pinning that is inherent to those packages up to the pins file of A.

OK, you are right, branches can be helpful to have in the manifest.
But I’m still confident that we must not put explicit version information into it.
They belongs into the `Package.pins` file.
That is enough to get reproducible builds.


(Martin Waitz) #11

Hi Boris,

OK, you are right, branches can be helpful to have in the manifest.
But I’m still confident that we must not put explicit version information into it.
They belongs into the `Package.pins` file.
That is enough to get reproducible builds.

By "explicit version information", do you mean that you shouldn't put a git revision hash in the manifest – only branches and version tags should be acceptable?

yes exactly.
BTW: the more I think about it, the more I like the possibility to specify a branch in the manifest.

I'd agree that the revision-based initializer is a marginal feature; normally your package should depend on a version range or is tracking a branch. That said, we can imagine that you might wind up needing to peg your dependency to a specific revision that isn't a version tag, and not track a moving branch, so this seemed like a fairly harmless feature to add for that case. What is your objection about supporting that?

The decision about whether to put this information in your pins or in your manifest should be driven by whether it's something your package requires, or is just a workflow choice. If you just want to temporarily stick to a specific revision of your dependency for workflow reasons, pinning is the right way to do that. If, however, your package currently requires that revision, and isn't going to work properly without it, then the manifest is the right way to express that. You'd want that specification to stick even if someone did `swift package update --repin` to get newer pinned revisions.

Well, I guess your package will also work with all commits following your special one.
Then I’d be enough to specify the branch: your special dependency version is already specified in the pins file and `swift package update` will only update to newer revisions.

So why would you (temporarily) want to stick to a specific version?
Because it happens to be the only compatible one at that time.
So even that is a workflow thing and not a „this is the one and only“.
And if you really really need to choose a specific commit which is in the past, then you can always create a special branch just for it.

I really like to have a clear separation between manifest and the pins file.
Otherwise I fear that inexperienced maintainers hard-code their dependencies to a specific version by accident.
I've seen too strict version specifications too often already (e.g. on npm) :wink:

— Martin

···

Am 31.01.2017 um 03:48 schrieb Rick Ballard <rballard@apple.com>:

On Jan 25, 2017, at 11:06 PM, Martin Waitz via swift-evolution <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> wrote:


(Rick Ballard) #12

This is not for the case where a package will work with all commits on a branch after the designated one. If that's the case, indeed a branch should be specified instead. This is for the case where there is a specific commit that's needed, and it doesn't have a version tag, and it either isn't on a current branch or the branch has moved past it to something incompatible.

You can't necessarily create a special branch for such a commit because you may not have permissions to push new branches to the repositories of your dependencies.

One thing to note is that if a maintainer does use this feature inappropriately, their package will fail once they tag it for release. One of the behaviors specified by this proposal is that if a package is found via a version tag, then if that package specifies any further dependencies via branch or revision, that is an error. The rule is that any versioned tag needs to fully specify all other dependencies via versioned tags. So a novice user will find out pretty quickly that they can't rely on revision specifiers for their actual releases.

In the future we hope to add a command to SwiftPM to help you prepare your package for a tagged release. That process would check this for you, among other things, so you'd find out about this problem before you created the tag.

  - Rick

···

On Jan 30, 2017, at 11:43 PM, Martin Waitz <tali@admingilde.org> wrote:

Hi Boris,

Am 31.01.2017 um 03:48 schrieb Rick Ballard <rballard@apple.com <mailto:rballard@apple.com>>:

On Jan 25, 2017, at 11:06 PM, Martin Waitz via swift-evolution <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> wrote:

OK, you are right, branches can be helpful to have in the manifest.
But I’m still confident that we must not put explicit version information into it.
They belongs into the `Package.pins` file.
That is enough to get reproducible builds.

By "explicit version information", do you mean that you shouldn't put a git revision hash in the manifest – only branches and version tags should be acceptable?

yes exactly.
BTW: the more I think about it, the more I like the possibility to specify a branch in the manifest.

I'd agree that the revision-based initializer is a marginal feature; normally your package should depend on a version range or is tracking a branch. That said, we can imagine that you might wind up needing to peg your dependency to a specific revision that isn't a version tag, and not track a moving branch, so this seemed like a fairly harmless feature to add for that case. What is your objection about supporting that?

The decision about whether to put this information in your pins or in your manifest should be driven by whether it's something your package requires, or is just a workflow choice. If you just want to temporarily stick to a specific revision of your dependency for workflow reasons, pinning is the right way to do that. If, however, your package currently requires that revision, and isn't going to work properly without it, then the manifest is the right way to express that. You'd want that specification to stick even if someone did `swift package update --repin` to get newer pinned revisions.

Well, I guess your package will also work with all commits following your special one.
Then I’d be enough to specify the branch: your special dependency version is already specified in the pins file and `swift package update` will only update to newer revisions.

So why would you (temporarily) want to stick to a specific version?
Because it happens to be the only compatible one at that time.
So even that is a workflow thing and not a „this is the one and only“.
And if you really really need to choose a specific commit which is in the past, then you can always create a special branch just for it.

I really like to have a clear separation between manifest and the pins file.
Otherwise I fear that inexperienced maintainers hard-code their dependencies to a specific version by accident.
I've seen too strict version specifications too often already (e.g. on npm) :wink: