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

More tweeks to renames #1554

Merged
merged 1 commit into from
Nov 28, 2018
Merged

More tweeks to renames #1554

merged 1 commit into from
Nov 28, 2018

Conversation

nrc
Copy link
Member

@nrc nrc commented Nov 28, 2018

r? @alexcrichton

This is mostly undoing some of the changes from previous PRs (with some renames). I had mistakenly made the tests unlike the actual packages downloaded, then made the code correct for the tests not real life. The actual meaningful change here is in src/rustup-dist/src/manifestation.rs, where we use the name of the component in the manifest to refer the name of a component in a package (where it matches the manifest, not the user-visible name).

@alexcrichton
Copy link
Member

Oh dear.. so the tests don't seem to be covering all these cases that are coming up? Is this something we are still confident in releasing just a week before the edition?

@nrc
Copy link
Member Author

nrc commented Nov 28, 2018

so the tests don't seem to be covering all these cases that are coming up?

There is test coverage, but it relies on the mocking matching the real life manifests, which is a non-trivial issue because the mocking is so complex.

Is this something we are still confident in releasing just a week before the edition?

My confidence is certainly less than 100%. I do think we should be able to either roll-back or fix rustup if we get bug reports in the next week. We usually do get lots of usage.

@alexcrichton alexcrichton merged commit f0a3d9a into rust-lang:master Nov 28, 2018
@alexcrichton
Copy link
Member

@nrc looks like you've got the stable branch management down, so whenever it's ready just ping me and I can do the release

@briansmith briansmith mentioned this pull request Feb 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants