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

Fix --without-components with subsetted components #119

Merged
merged 1 commit into from
Dec 21, 2022

Conversation

Mark-Simulacrum
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum commented Dec 21, 2022

It's not entirely clear what changed in 1.66, but rust-lang/rust#105755 shows that we are failing to run the install script with --without if there are subsetted component names.

This changes the behavior of the filtering to require an exact match rather than a partial match, which seems like the better way to go. It's not very clear to me that the previous behavior was actually a good idea.

r? @jyn514 or @pietroalbini
cc rust-lang/rust#105755

Will run a try build + dev-static nightly to check this actually works, but altering the 1.66 release with these changes did seem to do the right thing. (But I think the easiest thing is to merge this first and then do that later).

It's not entirely clear what changed in 1.66, but rust-lang/rust#105755
shows that we are failing to run the install script with --without if
there are subsetted component names.

This changes the behavior of the filtering to require an *exact* match
rather than a partial match, which seems like the better way to go. It's
not very clear to me that the previous behavior was actually a good
idea.
@rustbot
Copy link

rustbot commented Dec 21, 2022

Failed to set assignee to jyn514: cannot assign: HTTP status client error (403 Forbidden) for url (https://api.github.com/repos/rust-lang/rust-installer/issues/119/assignees)

Note: Only org members, users with write permissions, or people who have commented on the PR may be assigned.

@jyn514
Copy link
Member

jyn514 commented Dec 21, 2022

I trust pietro's review, not sure when I'll get a chance to look at this, but I do want to mention that rust-lang/rust#102565 might have regressed something by accident. I glanced at the code and this seems a lot less fragile than before though, so changing rust-installer instead of reverting seems good to me.

@Mark-Simulacrum
Copy link
Member Author

That PR landed in 1.67, so I think it can't be responsible for breakage in 1.66?

@Mark-Simulacrum Mark-Simulacrum merged commit 5b2eee7 into rust-lang:master Dec 21, 2022
@Mark-Simulacrum Mark-Simulacrum deleted the fix-install branch December 21, 2022 17:22
@jonhoo
Copy link
Contributor

jonhoo commented Dec 21, 2022

I do wish this could take into account renames known to Rustup so that the components would better match what people are used to, but not broken is better than broken 😅

@Mark-Simulacrum
Copy link
Member Author

Definitely happy to take a PR! If this wasn't a shell script I might even do it myself, but parsing TOML (I think necessary?) would be... very painful in shell.

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Dec 27, 2022
* `--without=component-a,component-b` now requires full component names.
  This fixes rust-lang#105755 (rust-lang/rust-installer#119).
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 29, 2022
…n514

Bump rust-installer

`--without=component-a,component-b` now requires full component names. This fixes rust-lang#105755 (rust-lang/rust-installer#119).

dev-static build succeeded, and installer script seems to work (see comment in thread).
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Jan 19, 2023
* `--without=component-a,component-b` now requires full component names.
  This fixes rust-lang#105755 (rust-lang/rust-installer#119).
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.

5 participants