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

Write manifest for MAJOR.MINOR channel to enable rustup convenience #76107

Merged
merged 2 commits into from
Oct 3, 2020

Conversation

carols10cents
Copy link
Member

This connects to rust-lang/rustup#794.

It's hard to remember if there have been patch releases for old versions
when you'd like to install the latest in a MAJOR.MINOR series.

When we're doing a stable release, we write duplicate manifests to
stable. With this change, only when we're doing a stable release, also
write duplicate manifests to MAJOR.MINOR to eventually enable rustup
(and any other tooling that builds Rust release URLs) to request, say,
1.45 and get 1.45.2 (assuming 1.45.2 is the latest available
1.45 and assuming that we never publish patch releases out of order).

I tested the best I could; it's a bit hard to get everything set up right
to be able to run the build-manifest tool. But I was able to run it with
a release of "1.45.2" and in addition to the files like channel-rust-1.45.2.toml
and channel-rust-stable.toml (and other manifests) that I got before this
change, I now get channel-rust-1.45.toml.

I believe this change to be safe to deploy as it does not change or remove
anything about manifests, just adds more. The actions in rust-central-station
that interact with manifests appear to use wildcards in such a way that it will
pick up these files without any problems.

There will need to be changes to rustup before rustup install 1.45 will work,
but we can wait for a stable release and stable patch releases to happen with this
change before making the rustup changes, so that we're not committing to anything
before we know it works.

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 30, 2020
@carols10cents carols10cents added the T-release Relevant to the release subteam, which will review and decide on the PR/issue. label Aug 30, 2020
@carols10cents
Copy link
Member Author

I feel like this is relevant to @Mark-Simulacrum and @pietroalbini 's interests.

@Mark-Simulacrum
Copy link
Member

This is basically adding a channel, like stable/nightly/beta for each 1.x release series, I think.

This seems like the right approach, mostly, though I do wonder if instead we should just patch rustup to auto-search 1.x.0, 1.x.1, 1.x.2, etc -- we'd at most make 2-3 extra requests for tiny files (and they could be HEAD requests at that). I do worry a bit that presumably when installing, the user wants rustup to consider 1.45 and 1.45.0 to be equivalent, whereas I think with just this patch we would not do so.

cc @kinnison as well since this is rustup-related

@carols10cents
Copy link
Member Author

carols10cents commented Aug 30, 2020

This seems like the right approach, mostly, though I do wonder if instead we should just patch rustup to auto-search 1.x.0, 1.x.1, 1.x.2, etc -- we'd at most make 2-3 extra requests for tiny files (and they could be HEAD requests at that).

I've considered that as well, and it's similar to what rustup does with searching for a nightly that contains all the desired components, so I don't think that approach is unreasonable either. I'm happy to close this and go work on that if that's the consensus.

One reason I think this is better is that tools other than rustup could use this functionality as well (although I don't know of anything else concretely that uses these manifests, linux distros? 🤷‍♀️ )

I do worry a bit that presumably when installing, the user wants rustup to consider 1.45 and 1.45.0 to be equivalent, whereas I think with just this patch we would not do so.

I'm used to rvm from Ruby land, and it supports saying, for example, rvm install 2.6 to get Ruby 2.6.5, so that's at least one data point for what people would expect.

@Mark-Simulacrum
Copy link
Member

Right, I mean that I would expect the following behavior:

$ rustup install 1.45.2
$ rustup install 1.45 # no-op

OTOH, that's not the behavior you get with nightly releases, even if nightly-YYYY-MM-DD is already installed as just "nightly" an install for it will download that nightly again. So maybe this is not actually what we want, I'm not sure.

@joshtriplett
Copy link
Member

joshtriplett commented Aug 30, 2020

This is awesome!

Regarding 1.45 vs 1.45.0: I would expect rustup install 1.45.0 to install the original release, not a point release. Perhaps someone may be trying to track down when something was introduced, for instance.

I would propose that if the user asks to install 1.45.0, and a subsequent point release exists, we display a warning that a newer point release exists (including a mention of rustup install 1.45), but go ahead and install 1.45.0 as requested.

@kinnison
Copy link
Contributor

kinnison commented Sep 3, 2020

When I discussed this with @pietroalbini previously, we were thinking about a 1.45.x type shape for this putative channel, but we did not consider attempting to link the concept together in rustup beyond permitting it to trust such a channel name. I guess we could, just as easily, support a 1.45 type shape though I'm not sure I've yet understood the exploration of potential "what I'd expect to happen when..." scenarios. When I was previously discussing it, this was more for CI than for humans.

@carols10cents
Copy link
Member Author

though I'm not sure I've yet understood the exploration of potential "what I'd expect to happen when..." scenarios.

Is there anything I could write up that would be helpful?

@kinnison
Copy link
Contributor

kinnison commented Sep 4, 2020

Is there anything I could write up that would be helpful?

If you want to write something up @carols10cents then I'd be grateful. Essentially we need a treatment of all the use-cases we're trying to solve here, and an indication of what goes on behind the scenes, so we can decide if (a) the only thing we need is the channel data, or (b) we'd need some rustup changes too.

For example, @Mark-Simulacrum 's use case in #76107 (comment) would need rustup changes and might then lead to confusion if 1.45.3 were released since it'd not clear what should then happen on rustup update.

However, @joshtriplett 's point about 1.45.0 (in #76107 (comment) ) is actually already the case (in that it would install that .0 release even if .1 etc exists) but the putative warning to emit would need changes to rustup too.

No matter what we pick, rustup will need teaching about how to parse the new version number shape since it forces the x.y.z shape right now, but understanding what it means when users install the x.y vs x.y.z in terms of what toolchains are available, what happens if you have 1.45.1 installed but run cargo +1.45 build for example, or if you have both 1.45.1 installed and 1.45 installed and then 1.45.2 is released and you run rustup update.

We need to understand how everything fits together to know for sure if we have a coherent and unsurprising set of behaviours proposed. Once we do, we can file appropriate issues against rustup to get those behaviours implemented and released :D

@Mark-Simulacrum
Copy link
Member

Hm, so I think for now I'd be fine with rustup behaving for these 1.45 toolchain files just like it does for stable/beta/nightly and the 1.x.y releases/beta-YYYY-MM-DD/nightly-YYYY-MM-DD releases. That is, we would expect each 1.x channel to only represent itself, and explicit installs of parts of that channel would not "see" that channel's prior installation. So this sequence:

Let's set the stage as stable=1.45.1.

rustup install 1.45.1 # downloads and installs
rustup install 1.45 # downloads and installs 1.45.1, again, but calls it 1.45
rustup install stable # downloads and installs 1.45.1, again, but calls it stable
# We release stable 1.45.2
rustup update # updates 1.45 to 1.45.2, stable to 1.45.2. Does not touch 1.45.1 installation.

If this behavior is something we can all agree is reasonable to start with, then I think this change to the manifest builder will probably do what we want, so we can merge this PR. I imagine once it's merged we'll want to go back and either re-generate or copy over manifests from 1.x.max(y) to 1.x for each x, so that we don't magically gain this support for 1.48+. But that's optional, and might not happen for a while if it needs manual work.

I imagine we probably want some sort of FCP here -- at least with T-release? I'll let discussion following this post play out a bit but will likely kick off fcp here soon-ish if my opinion (that we should proceed) doesn't change.

@kinnison
Copy link
Contributor

If this behavior is something we can all agree is reasonable to start with, then I think this change to the manifest builder will probably do what we want

So I'd be fine with the behviour; but it will also need rustup to understand/accept those channel names, otherwise you get:

dsilvers@listless:~$ rustup install 1.45
error: invalid toolchain name: '1.45'. Toolchain numbers tend to have three parts, e.g. 1.45.0
dsilvers@listless:~$

@carols10cents
Copy link
Member Author

So I'd be fine with the behviour; but it will also need rustup to understand/accept those channel names, otherwise you get:

Yes, that's what I was referring to in my PR description with:

There will need to be changes to rustup before rustup install 1.45 will work, but we can wait for a stable release and stable patch releases to happen with this change before making the rustup changes, so that we're not committing to anything before we know it works.

I imagine once it's merged we'll want to go back and either re-generate or copy over manifests from 1.x.max(y) to 1.x for each x, so that we don't magically gain this support for 1.48+. But that's optional, and might not happen for a while if it needs manual work.

Ah thank you for mentioning this, @Mark-Simulacrum! This did occur to me at some point after opening this but I kept forgetting to come back and say that 😅 I think it can just be a set of cp [1.[each minor].[last patch]] [1.[each minor] commands that I can write into a script to be reviewed and run by someone on the release team.

rustup install 1.45.1 # downloads and installs
rustup install 1.45 # downloads and installs 1.45.1, again, but calls it 1.45
rustup install stable # downloads and installs 1.45.1, again, but calls it stable
# We release stable 1.45.2
rustup update # updates 1.45 to 1.45.2, stable to 1.45.2. Does not touch 1.45.1 installation.

@Mark-Simulacrum also thank you for this write up! I think that all makes sense. I'd also add these scenarios that I think are consistent with what you have here and what would happen without rustup changes other than allowing x.y version numbers:

cargo +1.45 run # uses 1.45.2
rustup toolchain uninstall 1.45 # removes 1.45.2, does not touch stable or 1.45.1 installation
cargo +1.45 run # errors with 'toolchain 1.45 is not installed', even though 1.45.1 is available and stable is 1.45.2

@Mark-Simulacrum
Copy link
Member

Yes, the removal behavior is as I would expect (even if it's a bit unfortunate).

@rfcbot fcp merge

This is proposing that we create a new channel for each stable release (e.g., 1.45) intended to both avoid the annoying "1.45 is not a release" rustup error and for CI and other places where you may want to test that you compile with some 1.x Rust release but want to upgrade to patch releases if they get issued automatically.

Support in rustup will happen separately, but (AFAIK) landing this won't hurt that and we believe the rustup support won't need additional patches.

FCP is mostly because this is expanding our stable surface area, though I personally expect that we could stop issuing these at any point in the future if we felt it wasn't a good idea anymore.

@rfcbot
Copy link

rfcbot commented Sep 14, 2020

Team member @Mark-Simulacrum has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), 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 rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Sep 14, 2020
@joshtriplett
Copy link
Member

joshtriplett commented Sep 15, 2020 via email

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 15, 2020
@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Sep 17, 2020
@rfcbot
Copy link

rfcbot commented Sep 17, 2020

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Sep 17, 2020
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Sep 27, 2020
@bors
Copy link
Contributor

bors commented Sep 29, 2020

☔ The latest upstream changes (presumably #77145) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

This connects to rust-lang/rustup#794.

It's hard to remember if there have been patch releases for old versions
when you'd like to install the latest in a MAJOR.MINOR series.

When we're doing a stable release, we write duplicate manifests to
`stable`. With this change, only when we're doing a stable release, also
write duplicate manifests to `MAJOR.MINOR` to eventually enable rustup
(and any other tooling that builds Rust release URLs) to request, say,
`1.45` and get `1.45.2` (assuming `1.45.2` is the latest available
`1.45` and assuming that we never publish patch releases out of order).
@carols10cents carols10cents added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Oct 1, 2020
@carols10cents
Copy link
Member Author

r? @pietroalbini

I rebased to resolve the conflicts with your refactoring PR (omg it's so much nicer now!!) and tested, and I think it still does what I intend, but please re-review and make sure!!

@spastorino spastorino removed the to-announce Announce this issue on triage meeting label Oct 1, 2020
@carols10cents
Copy link
Member Author

@pietroalbini good catch! I changed it to look for "stable" instead :)

@pietroalbini
Copy link
Member

Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Oct 2, 2020

📌 Commit a8fe654 has been approved by pietroalbini

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 2, 2020
@pietroalbini pietroalbini added the relnotes Marks issues that should be documented in the release notes of the next release. label Oct 2, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 2, 2020
…as-schievink

Rollup of 8 pull requests

Successful merges:

 - rust-lang#75377 (Fix Debug implementations of some of the HashMap and BTreeMap iterator types)
 - rust-lang#76107 (Write manifest for MAJOR.MINOR channel to enable rustup convenience)
 - rust-lang#76745 (Move Wrapping<T> ui tests into library)
 - rust-lang#77182 (Add missing examples for Fd traits)
 - rust-lang#77251 (Bypass const_item_mutation if const's type has Drop impl)
 - rust-lang#77264 (Only use LOCAL_{STDOUT,STDERR} when set_{print/panic} is used. )
 - rust-lang#77421 (Revert "resolve: Avoid "self-confirming" import resolutions in one more case")
 - rust-lang#77452 (Permit ty::Bool in const generics for v0 mangling)

Failed merges:

r? `@ghost`
@bors bors merged commit ca0ff93 into rust-lang:master Oct 3, 2020
@rustbot rustbot added this to the 1.48.0 milestone Oct 3, 2020
@carols10cents carols10cents deleted the manifest-alias branch October 5, 2020 12:46
carols10cents added a commit to carols10cents/rust-central-station that referenced this pull request Oct 6, 2020
Now that rust-lang/rust#76107 has been merged,
new releases will also write their manifests to channel-rust-1.x.toml as
well as channel-rust-1.x.y.toml to enable `rustup install 1.48` to get
the latest patch release in a minor release series.

This commit adds an idempotent function to copy manifests for the last
patch release in every minor release series to the corresponding
minor manifest so that past minor versions will work with the rustup
functionality too.

This function should only need to be run once, but should be safe to run
more than once.
carols10cents added a commit to carols10cents/rust-central-station that referenced this pull request Oct 11, 2020
Now that rust-lang/rust#76107 has been merged,
new releases will also write their manifests to channel-rust-1.x.toml as
well as channel-rust-1.x.y.toml to enable `rustup install 1.48` to get
the latest patch release in a minor release series.

This commit adds an idempotent script to copy manifests and their
signatures for the last patch release in every minor release series to
the corresponding minor manifest files so that past minor versions will
work with the rustup functionality too.

This script should only need to be run once, but should be safe to run
more than once.

It starts at 1.8 because we don't have manifests for 1.0-1.7, and it ends
with 1.47 because 1.48 will be the first stable release to write out the
1.x channel manifest.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-release Relevant to the release subteam, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.