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

Allow rustup to handle unavailable packages #1063

Merged
merged 1 commit into from
Jun 22, 2017

Conversation

erickt
Copy link
Contributor

@erickt erickt commented Apr 13, 2017

Before this, if a package was unavailable (like nightly is at the moment),
it would error out with a message like "error: missing key: 'url'" because
at the moment a few of the rust-analysis packages didn't build. This resulted
in the channel-rust-nightly.toml to be created with blocks like this:

[pkg.rust-analysis.target.aarch64-apple-ios]
available = false

[pkg.rust-analysis.target.aarch64-linux-android]
available = false

[pkg.rust-analysis.target.aarch64-unknown-fuchsia]
available = false

[pkg.rust-analysis.target.aarch64-unknown-linux-gnu]
available = true
hash = "be50ffa6f94770929b53bae553977cb6d78b03506f033d14a7251c7b0cdb9035"
url = "https://static.rust-lang.org/dist/2017-04-13/rust-analysis-nightly-aarch64-unknown-linux-gnu.tar.gz"

rustup assumed that there'd always be a hash and url, which is not
the case when packages are unavaible. This patch then just updates
rustup to handle their absence.

Closes #1063

@erickt
Copy link
Contributor Author

erickt commented Apr 13, 2017

I forgot to mention that with this change the tests pass, and nightly now downloads. I'm not sure how to go about creating a test case for this bug though.

@Diggsey
Copy link
Contributor

Diggsey commented Apr 19, 2017

@erickt Thanks!
The test failures on windows and linux look legitimate. WRT testing this feature - if you look in rustup-mock, you can see that it generates several scenarios. A good way to test would be to add a missing package to one of the manifests in a scenario, and then add a test which tries to install from that manifest.

@brson
Copy link
Contributor

brson commented May 8, 2017

Seems ok. There's some forwards-compatibility risk here by relying on the url and hashes keys, with the addition of xz packages. That is, if the gz packages ever went away this logic would see the package as unavailable. I don't see the gz packages going away any time soon though.

So I'm ok accepting this package as is. Travis errors are legit though.

Edit: this comment was misreading the patch.

@bors
Copy link
Contributor

bors commented May 23, 2017

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

@brson
Copy link
Contributor

brson commented Jun 9, 2017

Hm, after reviewing this does seem to respect the 'available' flag in the manifest, not base its logic on the presence of the hash/url fields?

@brson
Copy link
Contributor

brson commented Jun 9, 2017

Oh right the original bug here is just that rustup errors in a weird way when a component is unavailable, because rcs generats manifests in a way I didn't expect. So the logic here is to avoid reading the url/hash fields when a component is unavailable to deal with the manifests that actually exist in the wild. Patch still seems fine to me.

@brson
Copy link
Contributor

brson commented Jun 9, 2017

I'm rebasing this.

@brson
Copy link
Contributor

brson commented Jun 9, 2017

Rebased and tested. Caught an error in the manifest stringification.

r? @Diggsey

@brson brson removed the inactive label Jun 9, 2017
@Diggsey
Copy link
Contributor

Diggsey commented Jun 10, 2017

Test failures are legit.

Before this, if a package was unavailable (like nightly is at the moment),
it would error out with a message like "error: missing key: 'url'" because
at the moment a few of the rust-analysis packages didn't build. This resulted
in the `channel-rust-nightly.toml` to be created with blocks like this:

```toml
[pkg.rust-analysis.target.aarch64-apple-ios]
available = false

[pkg.rust-analysis.target.aarch64-linux-android]
available = false

[pkg.rust-analysis.target.aarch64-unknown-fuchsia]
available = false

[pkg.rust-analysis.target.aarch64-unknown-linux-gnu]
available = true
hash = "be50ffa6f94770929b53bae553977cb6d78b03506f033d14a7251c7b0cdb9035"
url = "https://static.rust-lang.org/dist/2017-04-13/rust-analysis-nightly-aarch64-unknown-linux-gnu.tar.gz"
```

rustup assumed that there'd always be a `hash` and `url`, which is not
the case when packages are unavaible. This patch then just updates
rustup to handle their absence.
@brson
Copy link
Contributor

brson commented Jun 22, 2017

@bors r+

@bors
Copy link
Contributor

bors commented Jun 22, 2017

📌 Commit ea39fe0 has been approved by brson

@bors
Copy link
Contributor

bors commented Jun 22, 2017

⌛ Testing commit ea39fe0 with merge 60d2410...

bors added a commit that referenced this pull request Jun 22, 2017
Allow rustup to handle unavailable packages

Before this, if a package was unavailable (like nightly is at the moment),
it would error out with a message like "error: missing key: 'url'" because
at the moment a few of the rust-analysis packages didn't build. This resulted
in the `channel-rust-nightly.toml` to be created with blocks like this:

```toml
[pkg.rust-analysis.target.aarch64-apple-ios]
available = false

[pkg.rust-analysis.target.aarch64-linux-android]
available = false

[pkg.rust-analysis.target.aarch64-unknown-fuchsia]
available = false

[pkg.rust-analysis.target.aarch64-unknown-linux-gnu]
available = true
hash = "be50ffa6f94770929b53bae553977cb6d78b03506f033d14a7251c7b0cdb9035"
url = "https://static.rust-lang.org/dist/2017-04-13/rust-analysis-nightly-aarch64-unknown-linux-gnu.tar.gz"
```

rustup assumed that there'd always be a `hash` and `url`, which is not
the case when packages are unavaible. This patch then just updates
rustup to handle their absence.

Closes #1063
@bors
Copy link
Contributor

bors commented Jun 22, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: brson
Pushing 60d2410 to master...

@bors bors merged commit ea39fe0 into rust-lang:master Jun 22, 2017
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.

4 participants