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

Miri should be presented as miri-preview until it's stable #1728

Closed
cuviper opened this issue Mar 27, 2019 · 12 comments
Closed

Miri should be presented as miri-preview until it's stable #1728

cuviper opened this issue Mar 27, 2019 · 12 comments
Assignees

Comments

@cuviper
Copy link
Member

cuviper commented Mar 27, 2019

Describe the problem you are trying to solve

We would like the miri component to be presented as "miri-preview" until it has been deemed stable.

Describe the solution you'd like

Any advice on how to accomplish this would be fine. 😄

Notes

The component was renamed to miri-preview in rust-lang/rust#59236, so the current channel-rust-nightly.toml describes miri like this:

[pkg.miri-preview]
version = "0.1.0-nightly (72b4ee038 2019-03-11)"
[pkg.miri-preview.target.aarch64-unknown-linux-gnu]
available = true
url = "https://static.rust-lang.org/dist/2019-03-27/miri-nightly-aarch64-unknown-linux-gnu.tar.gz"
hash = "17c2b1ad92b2621460c6ae2bc3025b32d088c7ba15ef65c5da003645fd348d5d"
xz_url = "https://static.rust-lang.org/dist/2019-03-27/miri-nightly-aarch64-unknown-linux-gnu.tar.xz"
xz_hash = "e3683df7c976f029eec95f693cd302ddb003d01444db917613d20386595d0744"

# ... and other targets ...

[renames.miri]
to = "miri-preview"

As a result, you can use either rustup component add miri or miri-preview. This is fine, but the displayed "info" always calls it just miri, as does rustup component list. We ought to still present this as miri-preview until the tool is actually deemed stable.

I found that the other tools like rustfmt, which are stable, are defined the same way, with [pkg.rustfmt-preview] and [renames.rustfmt] to = "rustfmt-preview". We should continue to use the non-preview name for presenting these stable tools, which is why I think we may need some enhancement for miri's case, without affecting existing tool manifests.

Maybe we could just "flip" miri for now, defining [pkg.miri] and then rename the other way [renames.miri-preview] to = "miri". That feels like a backwards kludge, but I guess we wouldn't need rustup changes then.

cc @rust-lang/release @RalfJung @oli-obk @mati865

@mati865
Copy link
Contributor

mati865 commented Mar 27, 2019

I wanted to go miri-preview but I didn't found way to test it locally. In some places it was called miri and in the other it was miri-preview so I just made sure it works either way.
You can see it's history here: rust-lang/miri#520

@cuviper
Copy link
Member Author

cuviper commented Mar 27, 2019

@mati865 I think it's totally fine that it can be named either way. I just want the canonical name to be miri-preview until it's actually deemed stable.

@mati865
Copy link
Contributor

mati865 commented Mar 27, 2019

My PR was a bit overkill and would not mind reverting rename part as long as it doesn't break installation via rustup. I think it rustup would have to get fixed first: https://github.com/rust-lang/rustup.rs/blob/819ddda6b7fc1f18884f027f646cd8714f8c4e9a/src/lib.rs#L44 along with the tests. After next release it should work without rename in manifest.
I'm not decisive person by the way.

@kinnison
Copy link
Contributor

It's fine, we merged miri support in #1606 but I can easily change it over to default to miri-preview instead if you want that. It's a tiny change. I'm not certain when rustup 1.18.0 will be due though, just to warn you.

@kinnison kinnison self-assigned this Mar 27, 2019
@cuviper
Copy link
Member Author

cuviper commented Mar 27, 2019

I'm curious, what's the change? Looking at #1606, I tried:

-        "cargo-miri" => Some("miri"),
+        "cargo-miri" => Some("miri-preview"),

but AFAICS that only affects the install_msg. Maybe it needs something in Manifest too?

I'm not certain when rustup 1.18.0 will be due though, just to warn you.

Can we try to get it before this reaches stable? Rust 1.35 will be on 2019-05-23.

@cuviper
Copy link
Member Author

cuviper commented Mar 27, 2019

Also, I'm trusting your judgement whether this needs a change in rustup or build-manifest, maybe both.

@mati865
Copy link
Contributor

mati865 commented Mar 27, 2019

Definitely https://github.com/rust-lang/rust/pull/59236/files#diff-2ca914af4ccadf4a4cafbf6a7bc13dcdR423 has to go away if we want miri-preview to stick.
When I was debugging error: component manifest for 'miri' is corrupt I noticed miri is called miri-preview in Rust's bootstrap or miri in build-manifest and rustup so I went the "most compatible" way.

If @RalfJung agrees to move it back to miri-preview I don't see any issue with renaming everything to miri-preview as long as it doesn't break nightlies for too long.

@cuviper
Copy link
Member Author

cuviper commented Mar 27, 2019

As I said originally, I was thinking we might just keep the rename, but flipped. So it would still work to ask for either miri or miri-preview, and wouldn't break nightlies, but hopefully rustup would start displaying miri-preview.

@kinnison
Copy link
Contributor

I'm ready to give this a go (and PR the recommendation change from miri to miri-preview) if someone can tell me if the manifests are updated on nightly so that I can give it a go to check it's all good.

@cuviper
Copy link
Member Author

cuviper commented Mar 30, 2019

I tried actually using miri with beta, and found that it doesn't work at all, because xargo errors out when it sees the stable or beta channel. I opened rust-lang/rust#59544 to only let miri into the manifest on nightly.

I looked some more at flipping the package name and rename, but I'm not very confident that this will actually work. It looks to me like it's actually important for upgrade paths that the [pkg] name never changes. I guess we can choose to break nightly installs in that way, but it doesn't seem nice.

@kinnison
Copy link
Contributor

OK, in that case are we going to close this off, or do you still want some rustup changes?

@cuviper
Copy link
Member Author

cuviper commented Mar 30, 2019

I think we can leave it alone for now, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants