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

Support naming toolchains and --force #6

Merged
merged 1 commit into from
Jul 22, 2018
Merged

Conversation

Manishearth
Copy link
Contributor

It's easier to call cargo +master build or whatever. We may even want the default to be master when invoked with no commit hash.

Currently it's annoying to use this in CI since I have to recalculate the hash so that I can use the toolchain.

r? @kennytm`

@kennytm
Copy link
Owner

kennytm commented Jul 20, 2018

I'm fine with --name, but for CI use you could write:

MASTER=$(rustup-toolchain-install-master --github-token "$TOKEN") &&
cargo +"$MASTER" build

And with --name, I don't see a reason to introduce --force since you could simply run

rustup uninstall master

(this command always return 0 even with non-existing toolchain)

@Manishearth
Copy link
Contributor Author

-f just makes it easier to call locally though 😄

src/main.rs Outdated
@@ -238,6 +252,10 @@ fn run() -> Result<(), Error> {
exit(1);
}

if args.commits.len() > 1 && args.name.is_some() {
eprintln!("name argument can only be provided with a single commit");
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be followed by an early exit.

Cargo.toml Outdated
@@ -1,6 +1,6 @@
[package]
name = "rustup-toolchain-install-master"
version = "1.1.0"
version = "1.1.1"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make the version 1.2.0.

src/main.rs Outdated
} else {
Cow::Borrowed(toolchain.commit)
Cow::Borrowed(toolchain.name)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we run --alt --name somename we'll end up with a somename-alt toolchain, which might be surprising.

Perhaps move this logic to where the struct is constructed?

@Manishearth
Copy link
Contributor Author

Fixed

@kennytm kennytm merged commit dede4ae into kennytm:master Jul 22, 2018
@kennytm
Copy link
Owner

kennytm commented Jul 23, 2018

Published 1.2.0 to crates.io.

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