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

RFC for prepublication dependencies #1969

Merged
merged 7 commits into from Jul 13, 2017

Conversation

@aturon
Member

aturon commented Apr 10, 2017

This RFC proposes the concept of prepublication dependencies for Cargo. These dependencies augment a crate index (like crates.io) with new versions of crates that have not yet been published to the index. Dependency resolution then works as if those prepublished versions actually existed in the index. Prepublication dependencies thus act as a kind of "staging index".

Prepublication makes it possible to perform integration testing within a large crate graph before publishing anything to crates.io, and without requiring dependencies to be switched from the crates.io index to git branches. It can, to a degree, simulate an "atomic" change across a large number of crates and repositories, which can then actually be landed in a piecemeal, non-atomic fashion.

Rendered

@aturon aturon added the T-dev-tools label Apr 10, 2017

@aturon aturon self-assigned this Apr 10, 2017

@aturon

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Apr 10, 2017

Member

I'm guessing based on the CC's that this feature is something Servo is having trouble with?

Member

steveklabnik commented Apr 10, 2017

I'm guessing based on the CC's that this feature is something Servo is having trouble with?

@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Apr 10, 2017

Member

@steveklabnik Yes. The RFC spells things out from first principles, but the original context is rust-lang/cargo#2649

Member

aturon commented Apr 10, 2017

@steveklabnik Yes. The RFC spells things out from first principles, but the original context is rust-lang/cargo#2649

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Apr 10, 2017

Member

Thanks for writing up the RFC here @aturon! Overall the feature sounds great to me. Some notes I'd have are:

  • This'll has another drawback of using this feature where it breaks compatibility with all historical revisions of Cargo. This is a pretty standard drawback for Cargo features, however, and I think is certainly acceptable in this use case for a number of reasons.
  • Do we want to detail how the lockfile will be changed for this RFC? (e.g. what info is encoded, how it's loaded, etc). It's not obviously clear to me personally how it will be done right now, but I also don't think that it's crucial to be placed in this RFC.
  • In your "prepublishing a new minor version" example you had servo depend on foo from git, but that seems like it could be onerous if foo is deeply nested in servo's dependency graph. I suspect though that you could bump the version of foo and [prepublish] both foo and xml-rs to solve this use case, right?
  • I think there may still be a minor case where a publish can cause breakage, although I'm not sure with how much weight we want to consider the situation. The main worry here is that you use [prepublish] to point to a git repository but before publishing a breaking change happens (as some new piece of data arises). In your "prepublishing a new major version" it's similar to foo being updated for 0.10.0, but then after [prepublish] is committed a breaking change happens to xml-rs, breaking foo. I don't think there's much we can do to protect against that though? If you have a lock file you should be protected regardless from this level of breakage. (unsure if this wants to be mentioned)
Member

alexcrichton commented Apr 10, 2017

Thanks for writing up the RFC here @aturon! Overall the feature sounds great to me. Some notes I'd have are:

  • This'll has another drawback of using this feature where it breaks compatibility with all historical revisions of Cargo. This is a pretty standard drawback for Cargo features, however, and I think is certainly acceptable in this use case for a number of reasons.
  • Do we want to detail how the lockfile will be changed for this RFC? (e.g. what info is encoded, how it's loaded, etc). It's not obviously clear to me personally how it will be done right now, but I also don't think that it's crucial to be placed in this RFC.
  • In your "prepublishing a new minor version" example you had servo depend on foo from git, but that seems like it could be onerous if foo is deeply nested in servo's dependency graph. I suspect though that you could bump the version of foo and [prepublish] both foo and xml-rs to solve this use case, right?
  • I think there may still be a minor case where a publish can cause breakage, although I'm not sure with how much weight we want to consider the situation. The main worry here is that you use [prepublish] to point to a git repository but before publishing a breaking change happens (as some new piece of data arises). In your "prepublishing a new major version" it's similar to foo being updated for 0.10.0, but then after [prepublish] is committed a breaking change happens to xml-rs, breaking foo. I don't think there's much we can do to protect against that though? If you have a lock file you should be protected regardless from this level of breakage. (unsure if this wants to be mentioned)
@est31

This comment has been minimized.

Show comment
Hide comment
@est31

est31 Apr 11, 2017

Contributor

What if you want to both update the xml-rs branches for 0.9.2 and 0.8.1? I think it would be more useful to be able to use the known regexes in the [replace] section, like "xml-rs:^0.9" = { path = "../xml-rs" }. Maybe then for consistency reasons you shouldn't warn when you are replacing an already published version, but I think that advantage alone is not enough to introduce a third system to the two already existing ones [replace] and [paths], causing even more confusion about which one to use.

Contributor

est31 commented Apr 11, 2017

What if you want to both update the xml-rs branches for 0.9.2 and 0.8.1? I think it would be more useful to be able to use the known regexes in the [replace] section, like "xml-rs:^0.9" = { path = "../xml-rs" }. Maybe then for consistency reasons you shouldn't warn when you are replacing an already published version, but I think that advantage alone is not enough to introduce a third system to the two already existing ones [replace] and [paths], causing even more confusion about which one to use.

@nox

This comment has been minimized.

Show comment
Hide comment
@nox

nox Apr 11, 2017

Contributor

What if you want to both update the xml-rs branches for 0.9.2 and 0.8.1? I think it would be more useful to be able to use the known regexes in the [replace] section, like "xml-rs:^0.9" = { path = "../xml-rs" }.

Keys like in [replace] sound better indeed.

Maybe then for consistency reasons you shouldn't warn when you are replacing an already published version, but I think that advantage alone is not enough to introduce a third system to the two already existing ones [replace] and [paths], causing even more confusion about which one to use.

Are you arguing against the very concept of [prepublish]? We covered in spades why [replace] and [paths] are useless for invasive bumps across a big dependency graph, for example serde bumps.

Contributor

nox commented Apr 11, 2017

What if you want to both update the xml-rs branches for 0.9.2 and 0.8.1? I think it would be more useful to be able to use the known regexes in the [replace] section, like "xml-rs:^0.9" = { path = "../xml-rs" }.

Keys like in [replace] sound better indeed.

Maybe then for consistency reasons you shouldn't warn when you are replacing an already published version, but I think that advantage alone is not enough to introduce a third system to the two already existing ones [replace] and [paths], causing even more confusion about which one to use.

Are you arguing against the very concept of [prepublish]? We covered in spades why [replace] and [paths] are useless for invasive bumps across a big dependency graph, for example serde bumps.

@est31

This comment has been minimized.

Show comment
Hide comment
@est31

est31 Apr 11, 2017

Contributor

Are you arguing against the very concept of [prepublish]?

I don't know what you mean by "the very concept". Can you be more precise?

I am arguing against adding adding yet another mechanism when the already existing one can just be extended. If you add a new Cargo.toml section, that is a new mechanism for me. If you instead extend [replace], that is just extending an existing one. Allowing regexes into the replace category seems the most straightforward and least intrusive change to solve this IMO.

I just said that for consistency reasons, the only drawback of putting it into the [replace] section instead of a new one would be that you shouldn't warn when you are replacing an already published version, which is in turn different from the original [prepublish] proposal. Warning is not that important in my eyes, as in the end you can't push the crate in that form to crates.io either way, and entries in the [replace] section are all meant to be temporary, so all of them should be removed asap, not just the entries in [prepublish]. Because warning is not so important, I've said that it doesn't justify adding another mechanism.

We covered in spades why [replace] and [paths] are useless for invasive bumps across a big dependency graph, for example serde bumps.

Can you tell me of any advantage of making a prepublish category instead of extending the [replace] category with regexes like I suggested? All the points I've seen in the RFC, as much I could follow the argumentation, sometimes it was hard to follow, would be solved by my proposal as well, except for the warning like I described above.

Contributor

est31 commented Apr 11, 2017

Are you arguing against the very concept of [prepublish]?

I don't know what you mean by "the very concept". Can you be more precise?

I am arguing against adding adding yet another mechanism when the already existing one can just be extended. If you add a new Cargo.toml section, that is a new mechanism for me. If you instead extend [replace], that is just extending an existing one. Allowing regexes into the replace category seems the most straightforward and least intrusive change to solve this IMO.

I just said that for consistency reasons, the only drawback of putting it into the [replace] section instead of a new one would be that you shouldn't warn when you are replacing an already published version, which is in turn different from the original [prepublish] proposal. Warning is not that important in my eyes, as in the end you can't push the crate in that form to crates.io either way, and entries in the [replace] section are all meant to be temporary, so all of them should be removed asap, not just the entries in [prepublish]. Because warning is not so important, I've said that it doesn't justify adding another mechanism.

We covered in spades why [replace] and [paths] are useless for invasive bumps across a big dependency graph, for example serde bumps.

Can you tell me of any advantage of making a prepublish category instead of extending the [replace] category with regexes like I suggested? All the points I've seen in the RFC, as much I could follow the argumentation, sometimes it was hard to follow, would be solved by my proposal as well, except for the warning like I described above.

@nox

This comment has been minimized.

Show comment
Hide comment
@nox

nox Apr 11, 2017

Contributor

I don't understand how allowing regexps in [replace] change anything about the fact that [replace] has versions constraints that state that you can't replace a crate from crates.io with a crate that has a different version number, which was why [prepublish] was suggested as a solution.

Cf. rust-lang/cargo#2649.

Contributor

nox commented Apr 11, 2017

I don't understand how allowing regexps in [replace] change anything about the fact that [replace] has versions constraints that state that you can't replace a crate from crates.io with a crate that has a different version number, which was why [prepublish] was suggested as a solution.

Cf. rust-lang/cargo#2649.

@est31

This comment has been minimized.

Show comment
Hide comment
@est31

est31 Apr 11, 2017

Contributor

@nox maybe you have misunderstood what I have been proposing. Its a very hard to describe topic sometimes, I was similarly confused often when reading the RFC. I had read the thread you linked, its included in @aturon 's comment above.

The current functionality of [replace] looks like (given the "crate-name:version-specification" = specified-place syntax):

a) you may only specify precise versions of crates -- no "^0.1.0" allowed, only "0.1.0".
b) the crate at the specified place must have a version that matches the version specification in the key in [replace].
c) all occurences of the crate-version combo matching the one specified in the key in [replace] are replaced

The proposal in the linked thread was originally to get rid of b). My proposal is to keep b) and c) and change a) to:

a) you must specify a version range for your crate -- "^0.1.0" and "0.1.0" are both allowed, but omitting version info is not allowed, as well as bare * specifiers.

Let me explain it with the example from the RFC.

Instead of for the foo crate

[prepublish]
xml-rs = { git = "https://github.com/aturon/xml-rs", branch = "0.9.2" }

[dependencies]
xml-rs = "0.9.2"

and for the servo crate

[prepublish]
xml-rs = { git = "https://github.com/aturon/xml-rs", branch = "0.9.2" }

[dependencies]
foo = { git = "https://github.com/aturon/foo", branch = "fix-xml" }

You'd do for the foo crate:

[replace]
"xml-rs:0.9" = { git = "https://github.com/aturon/xml-rs", branch = "0.9.2" }

[dependencies]
xml-rs = "0.9.2"

and the servo crate

[dependencies]
foo = { git = "https://github.com/aturon/foo", branch = "fix-xml" }

or if you want to test the new xml-rs in all fitting deps of the servo crate (like in the example):

[replace]
"xml-rs:0.9" = { git = "https://github.com/aturon/xml-rs", branch = "0.9.2" }

[dependencies]
foo = { git = "https://github.com/aturon/foo", branch = "fix-xml" }
Contributor

est31 commented Apr 11, 2017

@nox maybe you have misunderstood what I have been proposing. Its a very hard to describe topic sometimes, I was similarly confused often when reading the RFC. I had read the thread you linked, its included in @aturon 's comment above.

The current functionality of [replace] looks like (given the "crate-name:version-specification" = specified-place syntax):

a) you may only specify precise versions of crates -- no "^0.1.0" allowed, only "0.1.0".
b) the crate at the specified place must have a version that matches the version specification in the key in [replace].
c) all occurences of the crate-version combo matching the one specified in the key in [replace] are replaced

The proposal in the linked thread was originally to get rid of b). My proposal is to keep b) and c) and change a) to:

a) you must specify a version range for your crate -- "^0.1.0" and "0.1.0" are both allowed, but omitting version info is not allowed, as well as bare * specifiers.

Let me explain it with the example from the RFC.

Instead of for the foo crate

[prepublish]
xml-rs = { git = "https://github.com/aturon/xml-rs", branch = "0.9.2" }

[dependencies]
xml-rs = "0.9.2"

and for the servo crate

[prepublish]
xml-rs = { git = "https://github.com/aturon/xml-rs", branch = "0.9.2" }

[dependencies]
foo = { git = "https://github.com/aturon/foo", branch = "fix-xml" }

You'd do for the foo crate:

[replace]
"xml-rs:0.9" = { git = "https://github.com/aturon/xml-rs", branch = "0.9.2" }

[dependencies]
xml-rs = "0.9.2"

and the servo crate

[dependencies]
foo = { git = "https://github.com/aturon/foo", branch = "fix-xml" }

or if you want to test the new xml-rs in all fitting deps of the servo crate (like in the example):

[replace]
"xml-rs:0.9" = { git = "https://github.com/aturon/xml-rs", branch = "0.9.2" }

[dependencies]
foo = { git = "https://github.com/aturon/foo", branch = "fix-xml" }
@carols10cents

This comment has been minimized.

Show comment
Hide comment
@carols10cents

carols10cents Apr 11, 2017

Member

Do other package managers in other languages have this problem, or is cargo the only one because of how it resolves duplicate dependencies? If it is a problem in other package managers, how do they deal with it?

I'm a little confused around this thought:

Dependency resolution then works as if those prepublished versions actually existed in the index.

Let's say a project is going through the process outlined in the RFC, so that they've submitted a patch to xml-rs and are waiting for it to be accepted and a new version to be published.

Once xml-rs version 0.9.2 is actually published, we can remove the [prepublish] sections, and Cargo will warn us that this needs to be done.

What if our PR to xml-rs has not yet been accepted, but xml-rs releases version 0.9.2? Then we'll get a warning that we should remove the [prepublish] section, but it doesn't actually include the patch we need, so we can't actually remove the [prepublish], right? Wouldn't that be confusing and require lots of knowledge about the states of multiple projects...?

Member

carols10cents commented Apr 11, 2017

Do other package managers in other languages have this problem, or is cargo the only one because of how it resolves duplicate dependencies? If it is a problem in other package managers, how do they deal with it?

I'm a little confused around this thought:

Dependency resolution then works as if those prepublished versions actually existed in the index.

Let's say a project is going through the process outlined in the RFC, so that they've submitted a patch to xml-rs and are waiting for it to be accepted and a new version to be published.

Once xml-rs version 0.9.2 is actually published, we can remove the [prepublish] sections, and Cargo will warn us that this needs to be done.

What if our PR to xml-rs has not yet been accepted, but xml-rs releases version 0.9.2? Then we'll get a warning that we should remove the [prepublish] section, but it doesn't actually include the patch we need, so we can't actually remove the [prepublish], right? Wouldn't that be confusing and require lots of knowledge about the states of multiple projects...?

@quodlibetor

This comment has been minimized.

Show comment
Hide comment
@quodlibetor

quodlibetor Apr 11, 2017

Re: other package managers: I deal with pip (python) extensively and it's possible to hack something kind of like this using the --find-links or --extra-index-url CLI flags (both of which extend the dependency search path). This works okay if you've got a good devpi infrastructure set up because devpi makes it easy to have per-user package indexes.

That only really works inside a corporate infrastructure, though. Pip does allow you to specify arbitrary URIs for dependencies, and since it pretty much just takes the first package it encounters which solves a given constraint (not distinguishing between pypi.python.org and any other source) you could theoretically use GitHub projects in the way that prepublish is being suggested.

Saying all that makes me think that the fundamental problem being solved by prepublish is one of merging different sources, and that the cargo warning/refusal to publish has to do more with levels of trust ("I trust crates.io more than GitHub") than it does actually prepublishing. For example, I could imagine a corporate environment that doesn't allow publishing if any dependencies are on crates.io, and which would therefore want the same kind of "allow me to test with external crates but give me a warning" behavior.

Re: other package managers: I deal with pip (python) extensively and it's possible to hack something kind of like this using the --find-links or --extra-index-url CLI flags (both of which extend the dependency search path). This works okay if you've got a good devpi infrastructure set up because devpi makes it easy to have per-user package indexes.

That only really works inside a corporate infrastructure, though. Pip does allow you to specify arbitrary URIs for dependencies, and since it pretty much just takes the first package it encounters which solves a given constraint (not distinguishing between pypi.python.org and any other source) you could theoretically use GitHub projects in the way that prepublish is being suggested.

Saying all that makes me think that the fundamental problem being solved by prepublish is one of merging different sources, and that the cargo warning/refusal to publish has to do more with levels of trust ("I trust crates.io more than GitHub") than it does actually prepublishing. For example, I could imagine a corporate environment that doesn't allow publishing if any dependencies are on crates.io, and which would therefore want the same kind of "allow me to test with external crates but give me a warning" behavior.

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Apr 11, 2017

Member

What if our PR to xml-rs has not yet been accepted, but xml-rs releases version 0.9.2? Then we'll get a warning that we should remove the [prepublish] section, but it doesn't actually include the patch we need, so we can't actually remove the [prepublish], right? Wouldn't that be confusing and require lots of knowledge about the states of multiple projects...?

This was exactly my first thought on reading this RFC.

Do other package managers in other languages have this problem, or is cargo the only one because of how it resolves duplicate dependencies? If it is a problem in other package managers, how do they deal with it?

IME, it has to do with the multiple resolution aspect; that is, this is because Servo wants to only keep one version of each dependency in the tree, for various reasons. This doesn't affect npm, for example, because:

  1. there's no -sys equivalent, that is, every package can be included multiple times
  2. JS users care a lot less about the downsides of having multiple versions of packages.

Ruby can't do multiple versions in the first place, which means that you end up just having to wait a while. Many ecosystems also don't have nearly as many packages and dependencies; that is, my impression is that npm's ecosystem has significantly more packages and dependencies generally than Ruby/rubygems do, and so it's less of an issue. Other ecosystems have even less.

Member

steveklabnik commented Apr 11, 2017

What if our PR to xml-rs has not yet been accepted, but xml-rs releases version 0.9.2? Then we'll get a warning that we should remove the [prepublish] section, but it doesn't actually include the patch we need, so we can't actually remove the [prepublish], right? Wouldn't that be confusing and require lots of knowledge about the states of multiple projects...?

This was exactly my first thought on reading this RFC.

Do other package managers in other languages have this problem, or is cargo the only one because of how it resolves duplicate dependencies? If it is a problem in other package managers, how do they deal with it?

IME, it has to do with the multiple resolution aspect; that is, this is because Servo wants to only keep one version of each dependency in the tree, for various reasons. This doesn't affect npm, for example, because:

  1. there's no -sys equivalent, that is, every package can be included multiple times
  2. JS users care a lot less about the downsides of having multiple versions of packages.

Ruby can't do multiple versions in the first place, which means that you end up just having to wait a while. Many ecosystems also don't have nearly as many packages and dependencies; that is, my impression is that npm's ecosystem has significantly more packages and dependencies generally than Ruby/rubygems do, and so it's less of an issue. Other ecosystems have even less.

@nox

This comment has been minimized.

Show comment
Hide comment
@nox

nox Apr 11, 2017

Contributor

This is because Servo wants to only keep one version of each dependency in the tree, for various reasons.

This is unrelated to [prepublish], in the case of serde, having multiple versions of it mostly just doesn't build, and rustc starts complaining that it wanted some type to implement Deserialize, while it only implemented Deserialize, but not the right one from the right serde version.

Contributor

nox commented Apr 11, 2017

This is because Servo wants to only keep one version of each dependency in the tree, for various reasons.

This is unrelated to [prepublish], in the case of serde, having multiple versions of it mostly just doesn't build, and rustc starts complaining that it wanted some type to implement Deserialize, while it only implemented Deserialize, but not the right one from the right serde version.

@Ericson2314

This comment has been minimized.

Show comment
Hide comment
@Ericson2314

Ericson2314 Apr 11, 2017

Contributor

With [replace], mirrors, and now this, we keep on beating around the bush of just allowing arbitrary sources/registries (I forget the Cargo terminology) to be combined using plain old map union.

Also, why doesn't [replace] with a version that doesn't yet exist work? That's the obvious choice for dealing with a breaking change.

Contributor

Ericson2314 commented Apr 11, 2017

With [replace], mirrors, and now this, we keep on beating around the bush of just allowing arbitrary sources/registries (I forget the Cargo terminology) to be combined using plain old map union.

Also, why doesn't [replace] with a version that doesn't yet exist work? That's the obvious choice for dealing with a breaking change.

@yodaldevoid

This comment has been minimized.

Show comment
Hide comment
@yodaldevoid

yodaldevoid Apr 12, 2017

@Ericson2314
From what I can tell, it looks like nothing, beyond the current implementation, is stopping [replace] from working with a version that doesn't yet exist on crates.io (or some other index.) The only problem with extending [replace] like this is that once that version does exist on crates.io there is no indication that the replace could now be removed. This is actually brought up in the RFC under the Detailed Design section under the second paragraph.

What if our PR to xml-rs has not yet been accepted, but xml-rs releases version 0.9.2? Then we'll get a warning that we should remove the [prepublish] section, but it doesn't actually include the patch we need, so we can't actually remove the [prepublish], right? Wouldn't that be confusing and require lots of knowledge about the states of multiple projects...?

I don't think this is actually a problem, or rather it is already a problem with our current tools. If [replace] is used to fix a bug in a dependency and test it, as is one of the current usecases of [replace], the author of the dependent crate has to watch the dependency closely to know when they can update to the next version. [prepublish] just adds a reminder to cargo when the new version is published that the dependency override may no longer be needed.

To tie these two points together: unless I am misunderstanding something, it seems we can succinctly define [prepublish] as [replace] where a version not in the index can be specified and a warning is given if the overridden version exists in the index already. It may be possible to get the same functionality as [prepublish] by extending [replace], but I am not sure that we want warnings to be thrown with the current uses of [repace]. Maybe there could be some sort of flag that says, "warn me if this [replace] is overriding a version in the index?"

yodaldevoid commented Apr 12, 2017

@Ericson2314
From what I can tell, it looks like nothing, beyond the current implementation, is stopping [replace] from working with a version that doesn't yet exist on crates.io (or some other index.) The only problem with extending [replace] like this is that once that version does exist on crates.io there is no indication that the replace could now be removed. This is actually brought up in the RFC under the Detailed Design section under the second paragraph.

What if our PR to xml-rs has not yet been accepted, but xml-rs releases version 0.9.2? Then we'll get a warning that we should remove the [prepublish] section, but it doesn't actually include the patch we need, so we can't actually remove the [prepublish], right? Wouldn't that be confusing and require lots of knowledge about the states of multiple projects...?

I don't think this is actually a problem, or rather it is already a problem with our current tools. If [replace] is used to fix a bug in a dependency and test it, as is one of the current usecases of [replace], the author of the dependent crate has to watch the dependency closely to know when they can update to the next version. [prepublish] just adds a reminder to cargo when the new version is published that the dependency override may no longer be needed.

To tie these two points together: unless I am misunderstanding something, it seems we can succinctly define [prepublish] as [replace] where a version not in the index can be specified and a warning is given if the overridden version exists in the index already. It may be possible to get the same functionality as [prepublish] by extending [replace], but I am not sure that we want warnings to be thrown with the current uses of [repace]. Maybe there could be some sort of flag that says, "warn me if this [replace] is overriding a version in the index?"

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Apr 13, 2017

Member

@est31

A critical difference for me between [prepublish] and [replace] is that replacments only work with existing versions, where prepublish is intended to work with nonexistent versions. This can come up quite a bit during a major version bump, for example.

For example consider a case where serde release 1.0.0. All crates that depend on serde likely now need a major version bump themselves. The work flow for this on Servo would look like:

  • Start at the base of the tree, start forking dependencies, adding 1.0.0 support, sending a PR.
  • As part of this PR, the major version of the lib is increased
  • When going down the dependency graph, previous dependencies may or may not have merged or published.

In this use case as you go down the dependency graph you can immediately start using [prepublish] to point to all your forks. All Cargo.toml in PRs then have the "correct" [dependencies] section (e.g. serde = "1.0.0" and a small [prepublish] section to be deleted soon afterwards. Eventually this can culminate in a PR to servo/servo which uses all the [prepublish] deps. At that point integration testing can be done to verify everything works, and PRs were sent in parallel to all the projects. Nothing had to block on someone else to take action.

In contrast to [replace], this is not possible today. The [replace] section restricts to same-version replacements, which means that with the version bumps also in flight the [replace] section cannot be used.

I see these as different enough use cases that a separate section is ok, and otherwise trying to reuse [replace] to me seems isomorphic (it'll just require some slightly odd syntax perhaps).

@carols10cents

That's a good point! Note that technically I believe builds won't break (because of lock files), but it does mean that if you follow through what the warning says the build may break (for a number of reasons). This aspect seems somewhat fundamental to the feature here though (the final state is unknown given the current state of a repository). I wonder, though, are you basically concluding that we shouldn't issue a warning at all?

Member

alexcrichton commented Apr 13, 2017

@est31

A critical difference for me between [prepublish] and [replace] is that replacments only work with existing versions, where prepublish is intended to work with nonexistent versions. This can come up quite a bit during a major version bump, for example.

For example consider a case where serde release 1.0.0. All crates that depend on serde likely now need a major version bump themselves. The work flow for this on Servo would look like:

  • Start at the base of the tree, start forking dependencies, adding 1.0.0 support, sending a PR.
  • As part of this PR, the major version of the lib is increased
  • When going down the dependency graph, previous dependencies may or may not have merged or published.

In this use case as you go down the dependency graph you can immediately start using [prepublish] to point to all your forks. All Cargo.toml in PRs then have the "correct" [dependencies] section (e.g. serde = "1.0.0" and a small [prepublish] section to be deleted soon afterwards. Eventually this can culminate in a PR to servo/servo which uses all the [prepublish] deps. At that point integration testing can be done to verify everything works, and PRs were sent in parallel to all the projects. Nothing had to block on someone else to take action.

In contrast to [replace], this is not possible today. The [replace] section restricts to same-version replacements, which means that with the version bumps also in flight the [replace] section cannot be used.

I see these as different enough use cases that a separate section is ok, and otherwise trying to reuse [replace] to me seems isomorphic (it'll just require some slightly odd syntax perhaps).

@carols10cents

That's a good point! Note that technically I believe builds won't break (because of lock files), but it does mean that if you follow through what the warning says the build may break (for a number of reasons). This aspect seems somewhat fundamental to the feature here though (the final state is unknown given the current state of a repository). I wonder, though, are you basically concluding that we shouldn't issue a warning at all?

@est31

This comment has been minimized.

Show comment
Hide comment
@est31

est31 Apr 13, 2017

Contributor

@alexcrichton

A critical difference for me between [prepublish] and [replace] is that replacments only work with existing versions, where prepublish is intended to work with nonexistent versions.

Yes, and that I propose to be fixed! I don't argue that the current [replace] is enough, its not. I'm not denying there is a limitation right now, please note that. It should be addressed! All I'm saying is that introducing a completely new system with [prepublish] is the wrong approach.

Can you read the a,b,c from my comment in #1969 (comment) and the sentence below? Again, I do not propose to keep [replace] restricted to existing versions. Also, I don't propose that [replace] should suddenly be able to violate the version requirements in the [dependencies] section. It should only apply the replacement if the version of the replacement matches the version criteria laid out in the [dependencies] section. So it will work greatly across major version bumps and similar. And if you only want one of your sub trees to get a replaced version, to gradually stage out that change, you can add [replace] to the Cargo.toml of the root crate of that subtree.

Contributor

est31 commented Apr 13, 2017

@alexcrichton

A critical difference for me between [prepublish] and [replace] is that replacments only work with existing versions, where prepublish is intended to work with nonexistent versions.

Yes, and that I propose to be fixed! I don't argue that the current [replace] is enough, its not. I'm not denying there is a limitation right now, please note that. It should be addressed! All I'm saying is that introducing a completely new system with [prepublish] is the wrong approach.

Can you read the a,b,c from my comment in #1969 (comment) and the sentence below? Again, I do not propose to keep [replace] restricted to existing versions. Also, I don't propose that [replace] should suddenly be able to violate the version requirements in the [dependencies] section. It should only apply the replacement if the version of the replacement matches the version criteria laid out in the [dependencies] section. So it will work greatly across major version bumps and similar. And if you only want one of your sub trees to get a replaced version, to gradually stage out that change, you can add [replace] to the Cargo.toml of the root crate of that subtree.

@est31

This comment has been minimized.

Show comment
Hide comment
@est31

est31 Apr 13, 2017

Contributor

Would you folks mind reading an alternative RFC that outlines my proposed extension of [replace] in a more detailed fashion? There seem to be some misconceptions about it. How open is your mind towards something different than [prepublish]?

Contributor

est31 commented Apr 13, 2017

Would you folks mind reading an alternative RFC that outlines my proposed extension of [replace] in a more detailed fashion? There seem to be some misconceptions about it. How open is your mind towards something different than [prepublish]?

@carols10cents

This comment has been minimized.

Show comment
Hide comment
@carols10cents

carols10cents Apr 14, 2017

Member

@alexcrichton

That's a good point! Note that technically I believe builds won't break (because of lock files), but it does mean that if you follow through what the warning says the build may break (for a number of reasons). This aspect seems somewhat fundamental to the feature here though (the final state is unknown given the current state of a repository). I wonder, though, are you basically concluding that we shouldn't issue a warning at all?

So @nox clarified in chat that in the imagined servo workflow, [prepublish] sections in Cargo.toml would never be merged to master. This made me more comfortable with it, since the person shepherding the branch where the [prepublish] crates are being integration tested presumably would be on top of whether or not the patch in the upstream projects that servo is waiting on had actually been merged or not. This seems ok to me.

There's nothing stopping anyone from merging [prepublish] things to master, though, and involving everyone on their project in the upgrade effort, but that's on them. I was concerned that that was the desired workflow and it seems chaotic to me, like we'd need another mechanism to check that a commit was in the version in the [prepublish] before recommending its removal, or explaining more that maybe you can remove [prepublish] now, but maybe the person on your team who submitted the upstream PR actually needs to go rebase their upstream PR if there's been a version bump since they submitted the patch and the patch didn't actually make it in there...

but if only one person on a team has to worry about that, and they know about the status of all 16 PRs they're waiting on and whether they've been released or not, then this seems better than not being able to integration test easily while you're waiting 🤷‍♀️

I feel like the documentation around this should be "if you're going to be waiting a long time for the upstream PRs to be merged and new versions to be cut, and you need to move forward, prefer git dependencies instead".

Member

carols10cents commented Apr 14, 2017

@alexcrichton

That's a good point! Note that technically I believe builds won't break (because of lock files), but it does mean that if you follow through what the warning says the build may break (for a number of reasons). This aspect seems somewhat fundamental to the feature here though (the final state is unknown given the current state of a repository). I wonder, though, are you basically concluding that we shouldn't issue a warning at all?

So @nox clarified in chat that in the imagined servo workflow, [prepublish] sections in Cargo.toml would never be merged to master. This made me more comfortable with it, since the person shepherding the branch where the [prepublish] crates are being integration tested presumably would be on top of whether or not the patch in the upstream projects that servo is waiting on had actually been merged or not. This seems ok to me.

There's nothing stopping anyone from merging [prepublish] things to master, though, and involving everyone on their project in the upgrade effort, but that's on them. I was concerned that that was the desired workflow and it seems chaotic to me, like we'd need another mechanism to check that a commit was in the version in the [prepublish] before recommending its removal, or explaining more that maybe you can remove [prepublish] now, but maybe the person on your team who submitted the upstream PR actually needs to go rebase their upstream PR if there's been a version bump since they submitted the patch and the patch didn't actually make it in there...

but if only one person on a team has to worry about that, and they know about the status of all 16 PRs they're waiting on and whether they've been released or not, then this seems better than not being able to integration test easily while you're waiting 🤷‍♀️

I feel like the documentation around this should be "if you're going to be waiting a long time for the upstream PRs to be merged and new versions to be cut, and you need to move forward, prefer git dependencies instead".

@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin

SimonSapin Apr 15, 2017

Contributor

What if our PR to xml-rs has not yet been accepted, but xml-rs releases version 0.9.2?

Good point. I think we should keep the warning but tweak its wording: not that the [prepublish] entry definitely should be removed, but to suggest checking if the crates.io version can be used instead.

Contributor

SimonSapin commented Apr 15, 2017

What if our PR to xml-rs has not yet been accepted, but xml-rs releases version 0.9.2?

Good point. I think we should keep the warning but tweak its wording: not that the [prepublish] entry definitely should be removed, but to suggest checking if the crates.io version can be used instead.

@Ericson2314

This comment has been minimized.

Show comment
Hide comment
@Ericson2314

Ericson2314 Apr 17, 2017

Contributor

@yodaldevoid thanks. It seems to me like [replace] and [prepublish] indeed have the same semantics but opposite warning conditions: [replace] would have a warning when the overridden version doesn't exist (on crates.io or a path dep) while [prepublish] would warn when the overridden version does exist.

Contributor

Ericson2314 commented Apr 17, 2017

@yodaldevoid thanks. It seems to me like [replace] and [prepublish] indeed have the same semantics but opposite warning conditions: [replace] would have a warning when the overridden version doesn't exist (on crates.io or a path dep) while [prepublish] would warn when the overridden version does exist.

@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Apr 17, 2017

Member

OK, time for some responses!

@alexcrichton

This'll has another drawback of using this feature where it breaks compatibility with all historical revisions of Cargo.

I think that's not too much of a problem here, given that we'll prevent you from publishing with this tag.

Do we want to detail how the lockfile will be changed for this RFC?

It'd be good to at least have a sketch of the design there, yes.

n your "prepublishing a new minor version" example you had servo depend on foo from git, but that seems like it could be onerous if foo is deeply nested in servo's dependency graph. I suspect though that you could bump the version of foo and [prepublish] both foo and xml-rs to solve this use case, right?

Yes.

I think there may still be a minor case where a publish can cause breakage, although I'm not sure with how much weight we want to consider the situation.

I'm not sure I quite understand the scenario. Could you try outlining it in a step-by-step fashion?


@est31

Is it fair to interpret the core of your idea as being that we could use the [replace] section with prepublish-style semantics when the version encountered does not currently exist?

If so, it's an interesting thought, and could be a nice simplification! The word "replace" is not ideal here, but we can deal with that separately.

Would you folks mind reading an alternative RFC that outlines my proposed extension of [replace] in a more detailed fashion? There seem to be some misconceptions about it. How open is your mind towards something different than [prepublish]?

That kind of thing is always welcome -- it's the whole point of the RFC process :-)

I think that your idea has merit, and would love to see it fleshed out some more. It doesn't have to be a whole RFC, though, maybe just a more detailed comment or gist? Whatever you prefer.


@carols10cents

Do other package managers in other languages have this problem, or is cargo the only one because of how it resolves duplicate dependencies?

Good question. Cargo is definitely somewhat different here because of the duplication strategy and the impact on static type checking. @wycats may be able to comment more.

Wouldn't that be confusing and require lots of knowledge about the states of multiple projects...?

It's a basic assumption here that, if you're using prepublish, you're already aware of the state of multiple projects, since you're basically making a "promise" about what an upstream crate is going to publish. Also, since you won't be able to publish uses to crates.io, it's restricted to a temporary situation. But yes, there's definitely potential for confusion, both in this case, and also for others working in a large project who don't realize that prepublish is being used, and may end up confused about which version is being pulled.

For mitigation, we can at least make the warning more broadly worded, to point you at these possibilities.

We could consider going further, and simply always printing something when you compile with prepublished deps. It's meant to be a short-term state, and that would help others working on the project who may not be aware.

I'll update the downsides section with some of these points.


@Ericson2314

With [replace], mirrors, and now this, we keep on beating around the bush of just allowing arbitrary sources/registries (I forget the Cargo terminology) to be combined using plain old map union.

That's certainly an alternative (and I'll update the section accordingly). But the design there is much more complex than you make it out to be, because if we manage this use case through a separate index, then we have a whole set of problems around managing that index. For example, when the crate is published upstream, you'll want to remove it from the index you layered on top. The proposed feature, OTOH, makes it very straightforward to see and manage exactly what is being added.

Also, why doesn't [replace] with a version that doesn't yet exist work? That's the obvious choice for dealing with a breaking change.

I'm not sure what you mean -- are you talking about replacing an existing version with a non-existent one, or something else?

Member

aturon commented Apr 17, 2017

OK, time for some responses!

@alexcrichton

This'll has another drawback of using this feature where it breaks compatibility with all historical revisions of Cargo.

I think that's not too much of a problem here, given that we'll prevent you from publishing with this tag.

Do we want to detail how the lockfile will be changed for this RFC?

It'd be good to at least have a sketch of the design there, yes.

n your "prepublishing a new minor version" example you had servo depend on foo from git, but that seems like it could be onerous if foo is deeply nested in servo's dependency graph. I suspect though that you could bump the version of foo and [prepublish] both foo and xml-rs to solve this use case, right?

Yes.

I think there may still be a minor case where a publish can cause breakage, although I'm not sure with how much weight we want to consider the situation.

I'm not sure I quite understand the scenario. Could you try outlining it in a step-by-step fashion?


@est31

Is it fair to interpret the core of your idea as being that we could use the [replace] section with prepublish-style semantics when the version encountered does not currently exist?

If so, it's an interesting thought, and could be a nice simplification! The word "replace" is not ideal here, but we can deal with that separately.

Would you folks mind reading an alternative RFC that outlines my proposed extension of [replace] in a more detailed fashion? There seem to be some misconceptions about it. How open is your mind towards something different than [prepublish]?

That kind of thing is always welcome -- it's the whole point of the RFC process :-)

I think that your idea has merit, and would love to see it fleshed out some more. It doesn't have to be a whole RFC, though, maybe just a more detailed comment or gist? Whatever you prefer.


@carols10cents

Do other package managers in other languages have this problem, or is cargo the only one because of how it resolves duplicate dependencies?

Good question. Cargo is definitely somewhat different here because of the duplication strategy and the impact on static type checking. @wycats may be able to comment more.

Wouldn't that be confusing and require lots of knowledge about the states of multiple projects...?

It's a basic assumption here that, if you're using prepublish, you're already aware of the state of multiple projects, since you're basically making a "promise" about what an upstream crate is going to publish. Also, since you won't be able to publish uses to crates.io, it's restricted to a temporary situation. But yes, there's definitely potential for confusion, both in this case, and also for others working in a large project who don't realize that prepublish is being used, and may end up confused about which version is being pulled.

For mitigation, we can at least make the warning more broadly worded, to point you at these possibilities.

We could consider going further, and simply always printing something when you compile with prepublished deps. It's meant to be a short-term state, and that would help others working on the project who may not be aware.

I'll update the downsides section with some of these points.


@Ericson2314

With [replace], mirrors, and now this, we keep on beating around the bush of just allowing arbitrary sources/registries (I forget the Cargo terminology) to be combined using plain old map union.

That's certainly an alternative (and I'll update the section accordingly). But the design there is much more complex than you make it out to be, because if we manage this use case through a separate index, then we have a whole set of problems around managing that index. For example, when the crate is published upstream, you'll want to remove it from the index you layered on top. The proposed feature, OTOH, makes it very straightforward to see and manage exactly what is being added.

Also, why doesn't [replace] with a version that doesn't yet exist work? That's the obvious choice for dealing with a breaking change.

I'm not sure what you mean -- are you talking about replacing an existing version with a non-existent one, or something else?

@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin

SimonSapin Apr 18, 2017

Contributor

We could consider going further, and simply always printing something when you compile with prepublished deps. It's meant to be a short-term state, and that would help others working on the project who may not be aware.

👍

I’ve been confused a couple times when starting work on something new in an existing repo because I forgot I still had paths or [replace] leftover from what I was working on before. So always warning sounds good.

Contributor

SimonSapin commented Apr 18, 2017

We could consider going further, and simply always printing something when you compile with prepublished deps. It's meant to be a short-term state, and that would help others working on the project who may not be aware.

👍

I’ve been confused a couple times when starting work on something new in an existing repo because I forgot I still had paths or [replace] leftover from what I was working on before. So always warning sounds good.

@wycats

This comment has been minimized.

Show comment
Hide comment
@wycats

wycats Apr 18, 2017

Contributor

We could consider going further, and simply always printing something when you compile with prepublished deps. It's meant to be a short-term state, and that would help others working on the project who may not be aware.

If possible, I'd prefer for this to be a (obvious) modification to the dependency line itself rather than yet another line at the end telling you you're using prepublished deps. We already have a different "look" for git deps, and I think it makes sense to find a way to call out the use of a prepublished dep in the line where the dep is used. It could also be printed even if the dep is "clean," probably.

Contributor

wycats commented Apr 18, 2017

We could consider going further, and simply always printing something when you compile with prepublished deps. It's meant to be a short-term state, and that would help others working on the project who may not be aware.

If possible, I'd prefer for this to be a (obvious) modification to the dependency line itself rather than yet another line at the end telling you you're using prepublished deps. We already have a different "look" for git deps, and I think it makes sense to find a way to call out the use of a prepublished dep in the line where the dep is used. It could also be printed even if the dep is "clean," probably.

@wycats

This comment has been minimized.

Show comment
Hide comment
@wycats

wycats Apr 18, 2017

Contributor

Another potentially viable approach would be to list out groups of dependencies with interesting characteristics at the very end. This could include things like yanked deps, deps using deprecated features, deps with capped lints, and prepublished deps. We could decide which of these groups to include by default (the capped lint one would, I suspect, produce more noise than value if on by default, but could be a nice thing to be allowed to enable with a flag).

Contributor

wycats commented Apr 18, 2017

Another potentially viable approach would be to list out groups of dependencies with interesting characteristics at the very end. This could include things like yanked deps, deps using deprecated features, deps with capped lints, and prepublished deps. We could decide which of these groups to include by default (the capped lint one would, I suspect, produce more noise than value if on by default, but could be a nice thing to be allowed to enable with a flag).

@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin

SimonSapin Apr 18, 2017

Contributor

If possible, I'd prefer for this to be a (obvious) modification to the dependency line itself

I think I misunderstood until I read your next comment, but here you mean line in the output of running Cargo, not line in a [dependencies] section of some Cargo.toml, right?

Contributor

SimonSapin commented Apr 18, 2017

If possible, I'd prefer for this to be a (obvious) modification to the dependency line itself

I think I misunderstood until I read your next comment, but here you mean line in the output of running Cargo, not line in a [dependencies] section of some Cargo.toml, right?

@wycats

This comment has been minimized.

Show comment
Hide comment
@wycats

wycats Apr 18, 2017

Contributor

@SimonSapin yes, I mean the output of running Cargo.

Contributor

wycats commented Apr 18, 2017

@SimonSapin yes, I mean the output of running Cargo.

@wycats

This comment has been minimized.

Show comment
Hide comment
@wycats

wycats Apr 18, 2017

Contributor

I think that the "on-by-default" groupings would be things that would happen rarely, which I think matches the expected use of prepublish dependencies. In contrast, capped lints would happen so often that they would simply become noise in the output. I'd be in favor of enabling all of the groupings by default in verbose mode, and also having a flag like --show-warnings that would take a list of groups to show (--show-warnings capped-lints,prepublish,yanked).

Contributor

wycats commented Apr 18, 2017

I think that the "on-by-default" groupings would be things that would happen rarely, which I think matches the expected use of prepublish dependencies. In contrast, capped lints would happen so often that they would simply become noise in the output. I'd be in favor of enabling all of the groupings by default in verbose mode, and also having a flag like --show-warnings that would take a list of groups to show (--show-warnings capped-lints,prepublish,yanked).

@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Apr 18, 2017

Member

@wycats I like both of these ideas! It seems like, for this particular RFC, we should probably just take the route of modifying the dependency line output. We can then separately consider the "interesting deps" idea as a first-class thing.

Member

aturon commented Apr 18, 2017

@wycats I like both of these ideas! It seems like, for this particular RFC, we should probably just take the route of modifying the dependency line output. We can then separately consider the "interesting deps" idea as a first-class thing.

@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Apr 24, 2017

Member

@est31, just to check in, do you have any sense for the timeframe on a comment/gist/counter-RFC? If you want to provide a slightly more detailed sketch, I'm happy to take care of the rest of the work to integrate it into the RFC (assuming that there's some consensus we want to head in that direction, of course).

Member

aturon commented Apr 24, 2017

@est31, just to check in, do you have any sense for the timeframe on a comment/gist/counter-RFC? If you want to provide a slightly more detailed sketch, I'm happy to take care of the rest of the work to integrate it into the RFC (assuming that there's some consensus we want to head in that direction, of course).

@est31

This comment has been minimized.

Show comment
Hide comment
@est31

est31 Apr 25, 2017

Contributor

@aturon is it enough when I write one till this week friday?

Contributor

est31 commented Apr 25, 2017

@aturon is it enough when I write one till this week friday?

@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Apr 25, 2017

Member

@est31 sounds great, thanks!

Member

aturon commented Apr 25, 2017

@est31 sounds great, thanks!

@carols10cents carols10cents added this to Merge proposed in Tracker Apr 26, 2017

@est31

This comment has been minimized.

Show comment
Hide comment
@est31

est31 Apr 28, 2017

Contributor

@aturon and everyone else, here it is, the gist: https://gist.github.com/est31/701d0e34f235f35b338ccfa2379ec620

I've ended up with a slightly different model, closer to this proposal, abandoning the ranges part of my proposal (that one was needless for the actual problems that required solving). There are differences in semantics to this proposal though, see the differences section of the gist.

Contributor

est31 commented Apr 28, 2017

@aturon and everyone else, here it is, the gist: https://gist.github.com/est31/701d0e34f235f35b338ccfa2379ec620

I've ended up with a slightly different model, closer to this proposal, abandoning the ranges part of my proposal (that one was needless for the actual problems that required solving). There are differences in semantics to this proposal though, see the differences section of the gist.

@Ericson2314

This comment has been minimized.

Show comment
Hide comment
@Ericson2314

Ericson2314 Apr 28, 2017

Contributor

@est31 yeah that looks like what I want—good job.

Contributor

Ericson2314 commented Apr 28, 2017

@est31 yeah that looks like what I want—good job.

@carols10cents

This comment has been minimized.

Show comment
Hide comment
@carols10cents

carols10cents Apr 28, 2017

Member

@est31 in the "Scenario: prepublishing a new minor version", it says "When compiling foo, Cargo will resolve the xml-rs dependency to 0.9.2, and retrieve the source from the specified git branch."

That means "when compiling foo as part of, and in the context of, compiling servo", correct? Since servo is the only Cargo.toml that knows about the git repo of foo that contains 0.9.2, according to the setup in this section. It doesn't look like foo with the Cargo.toml specified would compile independently... is that intentional or a mistake?

Also, under the "Cargo.lock behavior" section, it says "If there is one, the version that is being replaced should be put into Cargo.lock." Does "one" refer to "the version that is being replaced" or to "Cargo.lock"? I originally read this as "If there is a Cargo.lock", and there is always a Cargo.lock, right? Could you reword this sentence to clarify?

Regarding the unresolved question about the warnings, what does the output of cargo build look like today if a crate is replaced? (Sorry, I'm going to be lazy here and not set up a scenario to check myself, plus I'm guessing you might have a project in this state handy)

I kind of think both situations where a replaced version exists on crates.io and a replaced version does not exist on crates.io deserve to have clear output and be distinct from each other, just to avoid situations where you think you're using something other than what you're actually using. I'm still not sure (as I was unsure in my previous comments here) whether a warning would be useful and would be accurate enough in its suggestions to be useful in this new proposal either. 🤷‍♀️

Aside from these questions, I prefer your proposal-- I think the feature as you propose would be a lot more understandable and explainable, as well as being less complex as far as adding another special section to Cargo.toml goes.

Member

carols10cents commented Apr 28, 2017

@est31 in the "Scenario: prepublishing a new minor version", it says "When compiling foo, Cargo will resolve the xml-rs dependency to 0.9.2, and retrieve the source from the specified git branch."

That means "when compiling foo as part of, and in the context of, compiling servo", correct? Since servo is the only Cargo.toml that knows about the git repo of foo that contains 0.9.2, according to the setup in this section. It doesn't look like foo with the Cargo.toml specified would compile independently... is that intentional or a mistake?

Also, under the "Cargo.lock behavior" section, it says "If there is one, the version that is being replaced should be put into Cargo.lock." Does "one" refer to "the version that is being replaced" or to "Cargo.lock"? I originally read this as "If there is a Cargo.lock", and there is always a Cargo.lock, right? Could you reword this sentence to clarify?

Regarding the unresolved question about the warnings, what does the output of cargo build look like today if a crate is replaced? (Sorry, I'm going to be lazy here and not set up a scenario to check myself, plus I'm guessing you might have a project in this state handy)

I kind of think both situations where a replaced version exists on crates.io and a replaced version does not exist on crates.io deserve to have clear output and be distinct from each other, just to avoid situations where you think you're using something other than what you're actually using. I'm still not sure (as I was unsure in my previous comments here) whether a warning would be useful and would be accurate enough in its suggestions to be useful in this new proposal either. 🤷‍♀️

Aside from these questions, I prefer your proposal-- I think the feature as you propose would be a lot more understandable and explainable, as well as being less complex as far as adding another special section to Cargo.toml goes.

@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Apr 29, 2017

Member

Thanks @est31! The proposal makes sense to me, and has some nice upsides.

The downsides in my mind are:

  • Intent is less clear, since "replace" strongly connotes changing an existing crate version, not adding a new one. You can't signal that you intend prepublication.
  • Not warning when the crate is already published seems like a pretty significant downside to me, because if you failed to notice it, you could get very confused.
    • That said, it might well be mitigated by the fact that you can't publish a crate that uses [replace]

I also think these two downsides are highly related, and I wonder whether there's a compromise, in which we use the [replace] section, but somehow still signal intent to prepublish, e.g.:

"xml-rs:0.9.2" = { git = "https://github.com/aturon/xml-rs", branch = "0.9.2", prepublish = true }

What do you think?


Minor nit:

The requirement to go up the entire dependency chain with [prepublish] has been removed

But I think there's a misunderstanding here. The original RFC doesn't require you to put [prepublish] everywhere, it's just that if you're doing integration testing up the dependency tree, you'll need to use it at the top level of each place you want to test, which in the limit means going up the tree.

Member

aturon commented Apr 29, 2017

Thanks @est31! The proposal makes sense to me, and has some nice upsides.

The downsides in my mind are:

  • Intent is less clear, since "replace" strongly connotes changing an existing crate version, not adding a new one. You can't signal that you intend prepublication.
  • Not warning when the crate is already published seems like a pretty significant downside to me, because if you failed to notice it, you could get very confused.
    • That said, it might well be mitigated by the fact that you can't publish a crate that uses [replace]

I also think these two downsides are highly related, and I wonder whether there's a compromise, in which we use the [replace] section, but somehow still signal intent to prepublish, e.g.:

"xml-rs:0.9.2" = { git = "https://github.com/aturon/xml-rs", branch = "0.9.2", prepublish = true }

What do you think?


Minor nit:

The requirement to go up the entire dependency chain with [prepublish] has been removed

But I think there's a misunderstanding here. The original RFC doesn't require you to put [prepublish] everywhere, it's just that if you're doing integration testing up the dependency tree, you'll need to use it at the top level of each place you want to test, which in the limit means going up the tree.

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot May 22, 2017

@alexcrichton proposal cancelled.

rfcbot commented May 22, 2017

@alexcrichton proposal cancelled.

@alexcrichton alexcrichton added T-cargo and removed T-dev-tools labels May 22, 2017

@yodaldevoid

This comment has been minimized.

Show comment
Hide comment
@yodaldevoid

yodaldevoid May 22, 2017

@alexcrichon I'm a tad confused as to why you linked to that PR in your cancel. Was it a bit of copy-pasta?

yodaldevoid commented May 22, 2017

@alexcrichon I'm a tad confused as to why you linked to that PR in your cancel. Was it a bit of copy-pasta?

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton May 22, 2017

Member

Er sorry, my clipboard management is apparently real bad, this is the intended link

Member

alexcrichton commented May 22, 2017

Er sorry, my clipboard management is apparently real bad, this is the intended link

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton May 22, 2017

Member

@rfcbot fcp merge

Member

alexcrichton commented May 22, 2017

@rfcbot fcp merge

@carols10cents

This comment has been minimized.

Show comment
Hide comment
@carols10cents

carols10cents May 23, 2017

Member

i think rfcbot is confused...

Member

carols10cents commented May 23, 2017

i think rfcbot is confused...

@withoutboats withoutboats referenced this pull request May 23, 2017

Closed

Support new teams #142

@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon May 24, 2017

Member

Let's try again, I believe @dikaiosune may have made some updated!

@rfcbot fcp merge

Member

aturon commented May 24, 2017

Let's try again, I believe @dikaiosune may have made some updated!

@rfcbot fcp merge

@carols10cents

This comment has been minimized.

Show comment
Hide comment
@carols10cents

carols10cents May 24, 2017

Member

@aturon looks like no :(

Member

carols10cents commented May 24, 2017

@aturon looks like no :(

@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon May 25, 2017

Member

@rfcbot fcp merge

Member

aturon commented May 25, 2017

@rfcbot fcp merge

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot May 25, 2017

Team member @aturon has proposed to merge this. The next step is review by the rest of the tagged teams:

Concerns:

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

rfcbot commented May 25, 2017

Team member @aturon has proposed to merge this. The next step is review by the rest of the tagged teams:

Concerns:

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

Show outdated Hide outdated text/0000-cargo-prepublish.md
### Scenario: augmenting with a bugfix
Let's say that while developing `foo` we've got a lock file pointing to `xml-rs`
`0.8.0`, and we found the `0.8.0` branch of `xml-rs` that hasn't been touched

This comment has been minimized.

@matklad

matklad May 25, 2017

Member

Something seems wrong here? We've said that foo depends on 0.9.0, so there's no way we can see 0.8.0 in the lockfile.

@matklad

matklad May 25, 2017

Member

Something seems wrong here? We've said that foo depends on 0.9.0, so there's no way we can see 0.8.0 in the lockfile.

This comment has been minimized.

@alexcrichton

alexcrichton May 25, 2017

Member

Oops! Good catch, I'll update the RFC (just a minor error)

@alexcrichton

alexcrichton May 25, 2017

Member

Oops! Good catch, I'll update the RFC (just a minor error)

@matklad

This comment has been minimized.

Show comment
Hide comment
@matklad

matklad May 25, 2017

Member

Hello, @rfcbot !

@rfcbot reviewed

Member

matklad commented May 25, 2017

Hello, @rfcbot !

@rfcbot reviewed

alexcrichton added a commit to alexcrichton/cargo that referenced this pull request Jun 5, 2017

Implement the [augment] section of the manifest
This is an implementation of [RFC 1969] which adds a new section to top-level
manifests: `[augment]`. This section allows you to augment existing sources with
new versions of crates, possibly replacing the versions that already exist in
the source. More details about this feature can be found in the RFC itself.

[RFC 1969]: rust-lang/rfcs#1969

alexcrichton added a commit to alexcrichton/cargo that referenced this pull request Jun 5, 2017

Implement the [augment] section of the manifest
This is an implementation of [RFC 1969] which adds a new section to top-level
manifests: `[augment]`. This section allows you to augment existing sources with
new versions of crates, possibly replacing the versions that already exist in
the source. More details about this feature can be found in the RFC itself.

[RFC 1969]: rust-lang/rfcs#1969

alexcrichton added a commit to alexcrichton/cargo that referenced this pull request Jun 6, 2017

Implement the [augment] section of the manifest
This is an implementation of [RFC 1969] which adds a new section to top-level
manifests: `[augment]`. This section allows you to augment existing sources with
new versions of crates, possibly replacing the versions that already exist in
the source. More details about this feature can be found in the RFC itself.

[RFC 1969]: rust-lang/rfcs#1969
@withoutboats

This comment has been minimized.

Show comment
Hide comment
@withoutboats

withoutboats Jun 13, 2017

Contributor

@rfcbot concern "bikeshed the name"

I'd prefer a less esoteric name from "augment," I suggested some synonyms above, would love to get feedback or other ideas from anyone (of which I most like "patch").

Contributor

withoutboats commented Jun 13, 2017

@rfcbot concern "bikeshed the name"

I'd prefer a less esoteric name from "augment," I suggested some synonyms above, would love to get feedback or other ideas from anyone (of which I most like "patch").

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jun 14, 2017

Member

"patch" sounds great to me!

Member

alexcrichton commented Jun 14, 2017

"patch" sounds great to me!

@matklad

This comment has been minimized.

Show comment
Hide comment
@matklad

matklad Jun 14, 2017

Member

patch sounds great to me as well, but I would slightly prefer augment exactly because it is a bit more flavorful, which make it less ambiguous. I expect googling for cargo augment to be easier then for cargo patch. With patch, there's also a possibility of confusion with a literal patch as in git apply.

Member

matklad commented Jun 14, 2017

patch sounds great to me as well, but I would slightly prefer augment exactly because it is a bit more flavorful, which make it less ambiguous. I expect googling for cargo augment to be easier then for cargo patch. With patch, there's also a possibility of confusion with a literal patch as in git apply.

alexcrichton added a commit to alexcrichton/cargo that referenced this pull request Jun 15, 2017

Implement the [augment] section of the manifest
This is an implementation of [RFC 1969] which adds a new section to top-level
manifests: `[augment]`. This section allows you to augment existing sources with
new versions of crates, possibly replacing the versions that already exist in
the source. More details about this feature can be found in the RFC itself.

[RFC 1969]: rust-lang/rfcs#1969
@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Jul 3, 2017

Member

@rfcbot resolve "bikeshed the name"

Member

aturon commented Jul 3, 2017

@rfcbot resolve "bikeshed the name"

@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Jul 3, 2017

Member

I'm going into FCP manually, since @withoutboats is traveling and won't be able to formally resolve their concern in the near future.

Member

aturon commented Jul 3, 2017

I'm going into FCP manually, since @withoutboats is traveling and won't be able to formally resolve their concern in the near future.

alexcrichton added a commit to alexcrichton/cargo that referenced this pull request Jul 12, 2017

Implement the [patch] section of the manifest
This is an implementation of [RFC 1969] which adds a new section to top-level
manifests: `[patch]`. This section allows you to patch existing sources with
new versions of crates, possibly replacing the versions that already exist in
the source. More details about this feature can be found in the RFC itself.

[RFC 1969]: rust-lang/rfcs#1969
@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Jul 13, 2017

Member

Ok looks like 10 days have passed and not much new has come up during FCP, so I'm going to merge. An implementation of this can be found at rust-lang/cargo#4123

Member

alexcrichton commented Jul 13, 2017

Ok looks like 10 days have passed and not much new has come up during FCP, so I'm going to merge. An implementation of this can be found at rust-lang/cargo#4123

@alexcrichton alexcrichton merged commit a09a0c5 into rust-lang:master Jul 13, 2017

alexcrichton added a commit to alexcrichton/cargo that referenced this pull request Jul 18, 2017

Implement the [patch] section of the manifest
This is an implementation of [RFC 1969] which adds a new section to top-level
manifests: `[patch]`. This section allows you to patch existing sources with
new versions of crates, possibly replacing the versions that already exist in
the source. More details about this feature can be found in the RFC itself.

[RFC 1969]: rust-lang/rfcs#1969

bors added a commit to rust-lang/cargo that referenced this pull request Jul 18, 2017

Auto merge of #4123 - alexcrichton:augment, r=matklad
Implement the [patch] section of the manifest

This is an implementation of [RFC 1969] which adds a new section to top-level
manifests: `[patch]`. This section allows you to augment existing sources with
new versions of crates, possibly replacing the versions that already exist in
the source. More details about this feature can be found in the RFC itself.

[RFC 1969]: rust-lang/rfcs#1969
@bennofs

This comment has been minimized.

Show comment
Hide comment
@bennofs

bennofs Jul 30, 2017

There should be a better explanation at http://doc.crates.io/manifest.html#the-patch-section how this is different from [replace]. In particular, it was not clear to me from reading both the [replace] and the [patch] sections on that page how they differ.

bennofs commented Jul 30, 2017

There should be a better explanation at http://doc.crates.io/manifest.html#the-patch-section how this is different from [replace]. In particular, it was not clear to me from reading both the [replace] and the [patch] sections on that page how they differ.

@matklad

This comment has been minimized.

Show comment
Hide comment
@matklad

matklad Jul 30, 2017

Member

@bennofs the more thorough docs are on this page: http://doc.crates.io/specifying-dependencies.html#overriding-dependencies. [patch] is strictly more powerful then [replace] in that it allows to change the version of the replaced package as well. Hopefully when [patch] reaches stable we could remove [replace] mention from the main page to avoid confusion.

Member

matklad commented Jul 30, 2017

@bennofs the more thorough docs are on this page: http://doc.crates.io/specifying-dependencies.html#overriding-dependencies. [patch] is strictly more powerful then [replace] in that it allows to change the version of the replaced package as well. Hopefully when [patch] reaches stable we could remove [replace] mention from the main page to avoid confusion.

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Nov 3, 2017

Update to 1.21.0
Changelog:
Version 1.21.0 (2017-10-12)
==========================

Language
--------
- [You can now use static references for literals.][43838]
  Example:
  ```rust
  fn main() {
      let x: &'static u32 = &0;
  }
  ```
- [Relaxed path syntax. Optional `::` before `<` is now allowed in all contexts.][43540]
  Example:
  ```rust
  my_macro!(Vec<i32>::new); // Always worked
  my_macro!(Vec::<i32>::new); // Now works
  ```

Compiler
--------
- [Upgraded jemalloc to 4.5.0][43911]
- [Enabled unwinding panics on Redox][43917]
- [Now runs LLVM in parallel during translation phase.][43506]
  This should reduce peak memory usage.

Libraries
---------
- [Generate builtin impls for `Clone` for all arrays and tuples that
  are `T: Clone`][43690]
- [`Stdin`, `Stdout`, and `Stderr` now implement `AsRawFd`.][43459]
- [`Rc` and `Arc` now implement `From<&[T]> where T: Clone`, `From<str>`,
  `From<String>`, `From<Box<T>> where T: ?Sized`, and `From<Vec<T>>`.][42565]

Stabilized APIs
---------------

[`std::mem::discriminant`]

Cargo
-----
- [You can now call `cargo install` with multiple package names][cargo/4216]
- [Cargo commands inside a virtual workspace will now implicitly
  pass `--all`][cargo/4335]
- [Added a `[patch]` section to `Cargo.toml` to handle
  prepublication dependencies][cargo/4123] [RFC 1969]
- [`include` & `exclude` fields in `Cargo.toml` now accept gitignore
  like patterns][cargo/4270]
- [Added the `--all-targets` option][cargo/4400]
- [Using required dependencies as a feature is now deprecated and emits
  a warning][cargo/4364]


Misc
----
- [Cargo docs are moving][43916]
  to [doc.rust-lang.org/cargo](https://doc.rust-lang.org/cargo)
- [The rustdoc book is now available][43863]
  at [doc.rust-lang.org/rustdoc](https://doc.rust-lang.org/rustdoc)
- [Added a preview of RLS has been made available through rustup][44204]
  Install with `rustup component add rls-preview`
- [`std::os` documentation for Unix, Linux, and Windows now appears on doc.rust-lang.org][43348]
  Previously only showed `std::os::unix`.

Compatibility Notes
-------------------
- [Changes in method matching against higher-ranked types][43880] This may cause
  breakage in subtyping corner cases. [A more in-depth explanation is available.][info/43880]
- [rustc's JSON error output's byte position start at top of file.][42973]
  Was previously relative to the rustc's internal `CodeMap` struct which
  required the unstable library `libsyntax` to correctly use.
- [`unused_results` lint no longer ignores booleans][43728]

[42565]: rust-lang/rust#42565
[42973]: rust-lang/rust#42973
[43348]: rust-lang/rust#43348
[43459]: rust-lang/rust#43459
[43506]: rust-lang/rust#43506
[43540]: rust-lang/rust#43540
[43690]: rust-lang/rust#43690
[43728]: rust-lang/rust#43728
[43838]: rust-lang/rust#43838
[43863]: rust-lang/rust#43863
[43880]: rust-lang/rust#43880
[43911]: rust-lang/rust#43911
[43916]: rust-lang/rust#43916
[43917]: rust-lang/rust#43917
[44204]: rust-lang/rust#44204
[cargo/4123]: rust-lang/cargo#4123
[cargo/4216]: rust-lang/cargo#4216
[cargo/4270]: rust-lang/cargo#4270
[cargo/4335]: rust-lang/cargo#4335
[cargo/4364]: rust-lang/cargo#4364
[cargo/4400]: rust-lang/cargo#4400
[RFC 1969]: rust-lang/rfcs#1969
[info/43880]: rust-lang/rust#44224 (comment)
[`std::mem::discriminant`]: https://doc.rust-lang.org/std/mem/fn.discriminant.html
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment