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 first draft of top of tree dev proposal #1

Closed
wants to merge 5 commits into from

Conversation

neonichu
Copy link
Owner

@neonichu neonichu commented Dec 8, 2016

No description provided.

$ swift package edit bar --path ../bar
```

This will make `./Packages/bar` a symbolic link to the given path in the local filesystem and will ensure that the package manager will no longer be responsible for managing checkouts for the `bar` package, instead the user is responsible for managing the source control operations on their own. This is consistent with the current behaviour of `swift edit`. Using `swift package unedit` will also work unchanged.
Copy link

Choose a reason for hiding this comment

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

I think the symbolic link part is an implementation detail, we should first mention the problem this solves (and how). i.e. users usually have a checkout of the repository they work on and would like to attach that in this package graph.

We should also mention that the unedit part will not remove the contents at symlink and just remove the connection.

Also, if the path doesn't exist should we create a checkout?

Copy link
Owner Author

@neonichu neonichu Dec 8, 2016

Choose a reason for hiding this comment

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

Yah, I think creating the checkout on the user's behalf makes sense if it does not exist, yet.

)
```

Instead of checking out a tag, the package manager will check out the specified branch for the given package. If a package exists simultaneously with a specified version range and with a specified branch in the same package graph, an error will be emitted and checking out dependencies will fail. It is also an error to depend on a package referencing a branch transitively from a tagged package.
Copy link

Choose a reason for hiding this comment

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

I think we should allow if there are multiple references to a dependency and the root package contains the branch reference, and just emit warning in that case.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Maybe we could do a sanity check against the git history? If the branch is ahead of the version tag other occurrences of the dependency reference, we warn, otherwise we emit an error?


Instead of checking out a tag, the package manager will check out the specified branch for the given package. If a package exists simultaneously with a specified version range and with a specified branch in the same package graph, an error will be emitted and checking out dependencies will fail. It is also an error to depend on a package referencing a branch transitively from a tagged package.

Running `swift package update` will update packages referencing branches to their latest remote state. Running `swift package pin` will store the commit hash for the currently checked out revision in the `Pinfile`, so that other users of the package will receive the exact same revision if pinning is enabled.
Copy link

Choose a reason for hiding this comment

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

We will also want to store the branch name in pins file

We propose two solutions for these problems:

- extend the package manifest to allow specifying a branch instead of a version to support revlocked packages
- extend `swift package edit` to take an optional path argument to manually override the current behaviour
Copy link

Choose a reason for hiding this comment

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

This feels a little vague

@neonichu
Copy link
Owner Author

neonichu commented Dec 9, 2016

Split this into two proposals now as we originally intended.


## Introduction

This proposal adds enhancements to the package manifest to support development of packages without strict versioning. It is related to "top of tree" development, but described in a separate proposal, because the implementation is entirely orthogonal to it.

Choose a reason for hiding this comment

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

I would reword the second sentence to something like: "This is one of two features, along with , being proposed to enable use of SwiftPM to develop on "top of tree" of related packages."

@@ -0,0 +1,48 @@
# Package Manager Support for branches in the package manifest

Choose a reason for hiding this comment

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

I would leave off "in the package manifest", as that's a detail of our proposed solution, which is up for discussion. Maybe "Package Manager Support for Branch-Based Development"


## Motivation

The package manager currently supports packages depedencies which are strictly versioned according to semantic versioning. This works well for users of packages, but we still see some use cases where this hinders common development workflows:

Choose a reason for hiding this comment

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

"Dependencies" is misspelled.
Instead of "This works well for users of packages...", I would say something like "This is how a package's dependencies should be specified when that package is released, but this requirement hinders some development workflows:

The package manager currently supports packages depedencies which are strictly versioned according to semantic versioning. This works well for users of packages, but we still see some use cases where this hinders common development workflows:

- bootstrapping a new package which does not yet have a version at all
- allowing references to packages in branches instead of tags -- this is useful for working on revlocked packages

Choose a reason for hiding this comment

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

Allowing references to branches is the solution, not the broken workflow. I would rephrase the 2nd workflow to something like "Developing related packages in tandem in between releases, when one package may depend on the latest revision of another, which has not yet been tagged for release."


## Detailed Design

We will introduce a new initializer for `Package` which takes a branch instead of a version range:

Choose a reason for hiding this comment

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

Just to make it clear that this isn't replacing the prior initializer, I would call this a "second" initializer, not a "new" initializer. Also, this should be reworded slightly to make it clear that it's an initializer for the .Package dependency enum, not for the top level Package object.

)
```

Instead of checking out a tag, the package manager will check out the specified branch for the given package. If a package exists simultaneously with a specified version range and with a specified branch in the same package graph, an error will be emitted and checking out dependencies will fail. It is also an error to depend on a package referencing a branch transitively from a tagged package.
Copy link

@rballard rballard Dec 13, 2016

Choose a reason for hiding this comment

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

I would reword the first sentence to:
"Whenever dependencies are checked out or updated, if a dependency on a package specifies a branch instead of a version, the latest commit on that branch will be checked out for that package.

)
```

Instead of checking out a tag, the package manager will check out the specified branch for the given package. If a package exists simultaneously with a specified version range and with a specified branch in the same package graph, an error will be emitted and checking out dependencies will fail. It is also an error to depend on a package referencing a branch transitively from a tagged package.

Choose a reason for hiding this comment

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

I would reword the second sentence to:
"If a dependency on a package specifies a branch instead of a version range, all other dependencies on that package in the current package graph must specify the same branch, and not a version, or an error will be emitted and checking out dependencies will fail.

)
```

Instead of checking out a tag, the package manager will check out the specified branch for the given package. If a package exists simultaneously with a specified version range and with a specified branch in the same package graph, an error will be emitted and checking out dependencies will fail. It is also an error to depend on a package referencing a branch transitively from a tagged package.

Choose a reason for hiding this comment

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

The third sentence is hard to understand. I would rephrase, and move it into its own paragraph. Something like:
"While this feature is useful during development, a package's dependencies should be updated to point at versions instead of branches before that package is tagged for release. This is because a released package should provide a stable specification of its dependencies, and not break when a branch changes over time. To enforce this, it is an error if a package referenced by a version-based dependency specifies a branch in any of its dependencies. (Note that the converse is OK; a package referenced by a branch-based dependency may specify versions in its dependencies.)


Instead of checking out a tag, the package manager will check out the specified branch for the given package. If a package exists simultaneously with a specified version range and with a specified branch in the same package graph, an error will be emitted and checking out dependencies will fail. It is also an error to depend on a package referencing a branch transitively from a tagged package.

Running `swift package update` will update packages referencing branches to their latest remote state. Running `swift package pin` will store the commit hash for the currently checked out revision in the `Pinfile`, as well as the branch name, so that other users of the package will receive the exact same revision if pinning is enabled.

Choose a reason for hiding this comment

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

Let's discuss if this is the behavior we want from pinning. Should autopinning affect branch-based dependencies?

Copy link
Owner Author

Choose a reason for hiding this comment

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

I think it definitely should, the risk of a branch-based dependency breaking when being updated is much higher. Also this would be consistent with how other package managers handle this, e.g. CocoaPods, Bundler and Carthage.


## Alternative considered

None at this point.

Choose a reason for hiding this comment

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

One alternative we should mention would be storing the branch in a sidecar file that overrides the manifest instead of storing it in the manifest itself.


## Alternative considered

None at this point.

Choose a reason for hiding this comment

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

Another alternative would be to allow packages to be released while specifying branches for its dependencies. Though we did kind of address that above already (if you accept my proposed reword)

)
```

Whenever dependencies are checked out or updated, if a dependency on a package specifies a branch instead of a version, the latest commit on that branch will be checked out for that package. If a dependency on a package specifies a branch instead of a version range, all other dependencies on that package in the current package graph must specify the same branch, and not a version, or an error will be emitted and checking out dependencies will fail.

Choose a reason for hiding this comment

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

I think we should allow other dependencies to have versions and if root package mentions a branch it overrides the versioned dependencies.

Copy link
Owner Author

@neonichu neonichu Jan 12, 2017

Choose a reason for hiding this comment

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

I thought this might be confusing, but on the other hand you're right that it contradicts with the next paragraph and also with the intent of the proposal, because this would mean having to change a bunch of package manifests if a dependency is also used by leaf packages.

Choose a reason for hiding this comment

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

👍


Whenever dependencies are checked out or updated, if a dependency on a package specifies a branch instead of a version, the latest commit on that branch will be checked out for that package. If a dependency on a package specifies a branch instead of a version range, all other dependencies on that package in the current package graph must specify the same branch, and not a version, or an error will be emitted and checking out dependencies will fail.

While this feature is useful during development, a package's dependencies should be updated to point at versions instead of branches before that package is tagged for release. This is because a released package should provide a stable specification of its dependencies, and not break when a branch changes over time. To enforce this, it is an error if a package referenced by a version-based dependency specifies a branch in any of its dependencies.

Choose a reason for hiding this comment

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

To enforce this, it is an error if a package referenced by a version-based dependency specifies a branch in any of its dependencies.

Doesn't this contradicts the above point? ("all other dependencies on that package in the current package graph must specify the same branch")

Copy link
Owner Author

Choose a reason for hiding this comment

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

🤔 True, I think it makes sense to go with your suggestion for the previous paragraph and then this contradiction is resolved.

Choose a reason for hiding this comment

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

👍


While this feature is useful during development, a package's dependencies should be updated to point at versions instead of branches before that package is tagged for release. This is because a released package should provide a stable specification of its dependencies, and not break when a branch changes over time. To enforce this, it is an error if a package referenced by a version-based dependency specifies a branch in any of its dependencies.

Running `swift package update` will update packages referencing branches to their latest remote state. Running `swift package pin` will store the commit hash for the currently checked out revision in the `Pinfile`, as well as the branch name, so that other users of the package will receive the exact same revision if pinning is enabled.

Choose a reason for hiding this comment

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

nit: "pins file"


## Alternative considered

None at this point.

Choose a reason for hiding this comment

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

👍


## Proposed solution

As a solution to this problem, we propose to extend the package manifest to allow specifying a branch instead of a version to support revlocked packages and initial bootstrapping.
Copy link

Choose a reason for hiding this comment

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

Did we decide not to accept a revision here? Why not?

Copy link
Owner Author

Choose a reason for hiding this comment

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

We do now.

)
```

Whenever dependencies are checked out or updated, if a dependency on a package specifies a branch instead of a version, the latest commit on that branch will be checked out for that package. If a dependency on a package specifies a branch instead of a version range, it will override any versioned dependencies present in the current package graph that other packages might specify.
Copy link

Choose a reason for hiding this comment

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

I think we should call out just how dangerous that last sentence is (that it is clobbering requirements from other packages), and the use that to lead into next paragraph.

Copy link

Choose a reason for hiding this comment

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

Specifically, what we would like to address here is situations where a package has changed their development dependencies in such a way that, for example, the graph is unresolvable, but they don't get an error because something is in branch mode.


While this feature is useful during development, a package's dependencies should be updated to point at versions instead of branches before that package is tagged for release. This is because a released package should provide a stable specification of its dependencies, and not break when a branch changes over time. To enforce this, it is an error if a package referenced by a version-based dependency specifies a branch in any of its dependencies.

Running `swift package update` will update packages referencing branches to their latest remote state. Running `swift package pin` will store the commit hash for the currently checked out revision in the pins file, as well as the branch name, so that other users of the package will receive the exact same revision if pinning is enabled.
Copy link

Choose a reason for hiding this comment

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

What will we use the branch name for here? Diagnostics?


## Alternative considered

Storing the branch in a separate file that overrides the manifest, instead of the manifest itself.
Copy link

Choose a reason for hiding this comment

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

We should elaborate on this alternative and say why we didn't take it.

We should elaborate a tad on why we made it an error to publish a package with this (this was a subject of debate).

$ swift package edit bar --path ../bar
```

This allows users to manage their own checkout of the `bar` repository and will make the package manager use that instead of checking out a tagged version as it normally would. Concretely, this will make `./Packages/bar` a symbolic link to the given path in the local filesystem and will ensure that the package manager will no longer be responsible for managing checkouts for the `bar` package, instead the user is responsible for managing the source control operations on their own. This is consistent with the current behaviour of `swift edit`. Using `swift package unedit` will also work unchanged, but the checkout itself will not be deleted, only the symlink. If there is no existing checkout at the given filesystem location, the package manager will do an initial clone on the user's behalf.
Copy link

Choose a reason for hiding this comment

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

I think we should discuss why we put the symbolic link down.


Storing the branch in a separate file that overrides the manifest, instead of the manifest itself.

We decided to make publishing a package with unversioned dependencies an error, because a released package should provide a stable specification of its dependencies. A dependency on a branch could break at any time when the branch is being changed over time.

Choose a reason for hiding this comment

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

This is confusing, we don't have publishing feature


```bash
$ swift pin --branch <branchname>
$ swift pin --revision <revision>

Choose a reason for hiding this comment

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

We should mention somewhere that this will only take the 40 len hash and not other treeish

In addition to specifying a branch or revision in the manifest, we will also allow specifying it when pinning:

```bash
$ swift pin --branch <branchname>
Copy link

@ankitspd ankitspd Jan 23, 2017

Choose a reason for hiding this comment

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

This should be swift package pin <package-name> --branch <branch-name>


Whenever dependencies are checked out or updated, if a dependency on a package specifies a branch instead of a version, the latest commit on that branch will be checked out for that package. If a dependency on a package specifies a branch instead of a version range, it will override any versioned dependencies present in the current package graph that other packages might specify.

While this feature is useful during development, a package's dependencies should be updated to point at versions instead of branches before that package is tagged for release. This is because a released package should provide a stable specification of its dependencies, and not break when a branch changes over time. To enforce this, it is an error if a package referenced by a version-based dependency specifies a branch in any of its dependencies.

Choose a reason for hiding this comment

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

We should add some examples of graphs which are valid and not valid

@neonichu
Copy link
Owner Author

Superseded by apple#594

@neonichu neonichu closed this Jan 24, 2017
@neonichu neonichu deleted the package-manager-top-of-tree branch January 24, 2017 14:39
@chris-hatton
Copy link

chris-hatton commented Jan 25, 2017

Just a note to say it's great to see this feature coming in: it is critical to our team's ability to adopt the Swift Package Manager as we develop several projects along with their shared modules in parallel and want at least some of those dependencies to always remain 'latest'.

It would be even more useful if this feature could be driven by the branch-name or tag of the project repository i.e. if compiling from a branch named 'development', use the latest version of a dependency, and if compiling from a release-tagged or 'release/' prefixed branch, then use a pinned version of a dependency.

Or, would there be another emergent way of supporting this workflow?

@ddunbar
Copy link

ddunbar commented Jan 25, 2017

@chris-hatton we discussed that case, where you want the branch to be driven by something at the higher level (either the higher level package or the tool itself). I think this SE will at least let you easily implement it yourself w/ project specific scripts, and then we can discuss what more features it is worth supporting directly in the tool... how does that sound to you?

neonichu pushed a commit that referenced this pull request Nov 9, 2020
neonichu pushed a commit that referenced this pull request Nov 9, 2020
neonichu pushed a commit that referenced this pull request Apr 19, 2023
* structured-concurrency: improve wording and add examples

* [structured concurrency] updates from review 1

* cleanups

* groups dont suspend on spawn, they never did; more discussion

* fix a few remaining needless awaits

* introduce Spawned

* Kick off second review
neonichu pushed a commit that referenced this pull request Apr 19, 2023
* Add a draft proposal for parameter packs.

* Fix the type of postfix ... operator in the ambiguity example.

* update pitch (#1)

* Add missing SE proposal information.

* Replace the '...' syntax with 'repeat each'.

* Add more introductory explanation to the proposed solution.

* Update proposals/NNNN-parameter-packs.md

Co-authored-by: Remy Demarest <remy.demarest@gmail.com>

* Address editorial feedback

* Add pack iteration and pack element projection to the future directions.

* Move local value packs and add explicit pack syntax to future directions.

* Describe single-element tuple unwrapping under pack substitution.

* Simplify the description of same-type requirements involving parameter packs.

* Update proposals/NNNN-parameter-packs.md

Co-authored-by: Becca Royal-Gordon <beccadax@apple.com>

* Move the introduction above the table of contents.

* Specify trailing closure matching rules for parameter packs.

* Replace the term "type sequence" with "type list" to avoid confusion with the
Sequence protocol.

* Add more commentary on the `repeat each` syntax design.

* Minor editorial changes

* Describe how variadic generic functions interact with overload resolution.

* Update pitch links.

* Update 0393-parameter-packs.md

---------

Co-authored-by: Slava Pestov <sviatoslav.pestov@gmail.com>
Co-authored-by: Remy Demarest <remy.demarest@gmail.com>
Co-authored-by: Becca Royal-Gordon <beccadax@apple.com>
Co-authored-by: Xiaodi Wu <13952+xwu@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants