Added help for `rustup toolchain link` as documented in issue #954. by typesanitizer · Pull Request #1017 · rust-lang/rustup

typesanitizer · 2017-03-29T17:58:38Z

Also renamed TOOLCHAIN_INSTALL_HELP -> INSTALL_HELP for consistency.

…ng#954. * Also renamed TOOLCHAIN_INSTALL_HELP -> INSTALL_HELP for consistency.

jonhoo · 2017-03-29T18:24:54Z

src/rustup-cli/help.rs

+r"
+Symlinks a toolchain from a local directory specified by 'path' and
+gives it a custom name specified by 'toolchain' if it does not name a
+standard release channel.


I'm not sure I understand what this means? The text just below seems to imply that nightly is a perfectly fine name to use, but the text here seems to suggest that I can't use a "standard release channel" name?

(Okay, I don't fully understand all the code but here is what I think is correct.)
What it means is that if you give it a name such as a standard release channel, say nightly then it will parse and understand what you are trying to say (https://github.com/rust-lang-nursery/rustup.rs/blob/master/src/rustup/config.rs#L388).
You can certainly use nightly but that would mean that your actual nightly compiler (if you have one) will be replaced by the one at 'path'.
If you give it some unknown name which it doesn't recognize, say nightly1, then it will think that you are trying to give a custom name to the toolchain and will oblige.

The call order is toolchain_link (in rustup-cli/rustup-mode.rs) -> get_toolchain (in rustup/config.rs) -> Toolchain::from (in rustup/Toolchain.rs) -> resolve_toolchain (in rustup/config.rs, linked earler) -> PartialToolchainDesc::from_str (in rustup-dist/src/dist.rs).

Ah, I see. Then I think the text should be clearer about this. How about:

Symlinks a toolchain from a local directory specified by 'path' and gives it a custom name specified by 'toolchain'. If toolchain matches the name of a standard release channel such as stable, nightly, 1.8.0, etc., then path will be used in place of the standard binaries for those toolchains. For example, if a toolchain symlink is set up called nightly, any directory with a nightly override will now also use the new symlink.

Also, I notice the text currently says

For more information see rustup help toolchain
Does rustup help toolchain actually give more information at the moment?

How about: ...

Good point. Your explanation is much clearer than mine.

Does rustup help toolchain actually give more information at the moment?

rustup help toolchain has a full explanation of the toolchain name being split as <channel>[-<date>][-<host>] alongwith a couple of examples and the meanings of the individual terms. This is also referred to by rustup help installso I thought I should follow suit.

In fact, I wonder if we should also repeat some, if not all, of TOOLCHAIN_LINK_HELP in rustup help toolchain as well (if that's not already the case).

if that's not already the case

Currently, TOOLCHAIN_HELP (used for rustup help toolchain) only tells what each subcommand does in a succinct manner and each subcommand is responsible for its own documentation (or lack thereof). The current documentation for link inside TOOLCHAIN_HELP is just:

SUBCOMMANDS:
...
link Create a custom toolchain by symlinking to a directory
...
Toolchain names that don't name a channel instead can be used to name
custom toolchains with the rustup toolchain link command. This can
also be used to symlink toolchains from local builds. For more
information see rustup toolchain help link.

I feel that this is sufficient information to indicate to the user whether toolchain link is something they want to use, although the wording could be improved to make the dual purpose (overwriting or aliasing) clearer. Just making the utility clearer doesn't require the full details; e.g. how the path is to be specified.

brson · 2017-03-31T01:53:38Z

src/rustup-cli/help.rs

-custom toolchains with the `rustup toolchain link` command.";
+custom toolchains with the `rustup toolchain link` command. This can
+also be used to symlink toolchains from local builds. For more
+information see `rustup toolchain help link`.";


These two sentences seem slightly redundant. What about something like "rustup can also manage symlinked local toolchain builds, which is often used for developing Rust itself. For more information see rustup toolchain help link.

brson · 2017-03-31T01:55:52Z

src/rustup-cli/help.rs

+
+'path' is specified as `/path/to/rust/build/$triple/stage1` where
+'$triple' should be replaced with the desired triple. Only a build of
+`rustc` is required to be present at the given 'path'.


toolchain link can work with toolchains from any source, and this advice is specifically about linking the toolchain directly from the build directory. Can you clarify? Maybe like, "When used for development of Rust itself, toolchains can be linked directly out of the build directory, specifying path as ...".

brson · 2017-03-31T01:56:54Z

Thanks so much @theindigamer. I left a few comments.

Wrt the semantics around whether its ok to reuse names of release channels, I don't recall what rustup does and am happy to merge whatever you and @jonhoo agree on.

typesanitizer · 2017-03-31T18:10:24Z

@brson , @jonhoo
Thanks for all the feedback.
I tested the semantics and it turns out that you cannot name a custom toolchain with a standard name such as nightly; rustup will throw an error (my bad, I didn't do this earlier).

$ rustup toolchain link stable build/x86_64-unknown-linux-gnu/stage2
error: invalid custom toolchain name: 'stable-x86_64-unknown-linux-gnu'

That said, here are all the changes I propose:

At the end of toolchain help

rustup can also manage symlinked local toolchain builds, which are
often used to for developing Rust itself. For more information see
rustup toolchain help link.";

In toolchain link help

Symlinks a toolchain from a local directory specified by 'path' and
gives it a custom name specified by 'toolchain'.

'toolchain' is the custom name to be assigned to your toolchain. Any
name is permitted as long as it does not fully match an initial
substring of a standard release channel. For example, you can use
the names 'latest' or '2017-04-01' but you cannot use 'stable' or
'beta-i686' or 'nightly-x86_64-unknown-linux-gnu'.

'path' specifies the directory where the binaries and libraries for
the custom toolchain can be found. For example, when used for
development of Rust itself, toolchains can be linked directly out of
the build directory. After building, you can test out different
compiler versions as follows:
$ rustup toolchain list
stable-x86_64-unknown-linux-gnu (default)
$ rustup toolchain link latest-stage1 build/x86_64-unknown-linux-gnu/stage1
$ rustup toolchain list
stable-x86_64-unknown-linux-gnu (default)
latest-stage1
$ rustup toolchain default latest-stage1
If you now compile a crate with default settings, the custom
toolchain 'latest-stage1' will be used.

(I've used four spaces for indenting the code; Github put that in a code block for display.)

If you folks think the explanation is satisfactory, I'll add a commit with the changes
and update the PR.

jonhoo · 2017-03-31T18:48:13Z

@theindigamer good catch! The updated text looks pretty good, though I'd suggest two changes:

"Symlinks a toolchain [..] and gives it a custom name specified by 'toolchain'.", and "'toolchain' is the custom name to be assigned to your toolchain" coming right after each other seems a bit redundant. You can probably remove one of them.
I would remove the rustup toolchain list invocations from the example -- at least I personally don't feel as though they add much.

typesanitizer · 2017-03-31T20:49:26Z

Alright. Updated text for toolchain link help (I removed the top bit as there is already a message "Create a custom toolchain by symlinking to a directory" present):

'toolchain' is the custom name to be assigned to the new toolchain.
Any name is permitted as long as it does not fully match an initial
substring of a standard release channel. For example, you can use
the names 'latest' or '2017-04-01' but you cannot use 'stable' or
'beta-i686' or 'nightly-x86_64-unknown-linux-gnu'.

'path' specifies the directory where the binaries and libraries for
the custom toolchain can be found. For example, when used for
development of Rust itself, toolchains can be linked directly out of
the build directory. After building, you can test out different
compiler versions as follows:
$ rustup toolchain link latest-stage1 build/x86_64-unknown-linux-gnu/stage1
$ rustup override set latest-stage1
If you now compile a crate in the current directory, the custom
toolchain 'latest-stage1' will be used.

EDITS:

"your toolchain" -> "the new toolchain"
"default" -> "override set"
~~"the default settings"~~
fixed formatting

jonhoo · 2017-03-31T21:08:47Z

Any particular reason you want to set the new one as default? Could we instead use override set? Also, can we s/your toolchain/the new toolchain/.

typesanitizer · 2017-03-31T21:22:21Z

@jonhoo , good point. I didn't think about the default point. I've updated the comment.
(I didn't want to keep pasting it over and over again and increase the visual clutter.)

jonhoo · 2017-03-31T21:30:52Z

I think that means you can also remove " with default settings". With that change I'm happy :)

typesanitizer · 2017-03-31T21:50:08Z

I have the bit about default settings because someone might have declared an explicit dependency on a rustc version like here (which is not a default setting) and the toolchain change might potentially conflict with that, in which case compiling wouldn't work. I'm not sure how cargo handles that kind of error.

EDIT: I recognize that it is just a PR which has not been merged but I thought there might be some other way (which I cannot think of) where a user has some non-default settings so that they end up some specific compiler, which will prevent the override from working.

EDIT 2: I also concede that the wording "with default settings" is kind of bad and compresses too much meaning into few words. Well, if a user has a weird build system, then they probably know if and when override set does not work. Done, removed.

bors · 2017-04-02T09:21:13Z

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

…t-lang#954).

jonhoo · 2017-04-03T19:04:58Z

@theindigamer the latest draft of the text looks good. I see you also pushed some checks for the existence of rustc inside there. Is that truly the only valid path structure? Is there a particular reason for why toolchain link doesn't just take a rustc, but instead the stage1 directory?

typesanitizer · 2017-04-03T20:00:15Z

@jonhoo

Is that truly the only valid path structure?

This is a minimal check. There may be additional folders such as etc and share (your local installations of stable/nightly rust probably have these). But bin and lib are definitely required.

Is there a particular reason for why toolchain link doesn't just take a rustc, but instead the stage1 directory?

The .so files in lib are required for compilation; it will fail if you delete all of them.

jonhoo · 2017-04-03T20:03:55Z

Okay, you've got my r+, even though you don't need it :p
@brson r?

brson · 2017-04-05T00:53:03Z

src/rustup-cli/rustup_mode.rs

+    pathbuf.push("bin");
+    try!(utils::assert_is_directory(&pathbuf));
+    pathbuf.push("rustc");
+    try!(utils::assert_is_file(&pathbuf));


I'm ok with adding this validation, but will you sink it into the toolchain.install_from_dir call below? This file is purely for argument parsing.

@brson Done. I've added it to the top of the function as install_from_dir may be used in the future with link=false but it would still require checking the directory structure correctly in that case.

Whoops, sorry I had to change one line (remove the added dependency on PathBuf) so I rebased locally. (That's why the first commit link below is dead.)

brson · 2017-04-05T00:53:35Z

This looks great to me. I just added one nit about where to do the new validation. Thanks @theindigamer and @jonhoo.

brson · 2017-04-08T01:40:09Z

Looks like there's a merge conflict now @jonhoo. Can you rebase?

jonhoo · 2017-04-08T15:46:23Z

@theindigamer owns this, so he has to be the one to do it :)

brson · 2017-04-08T18:48:52Z

Lol sorry @jonhoo .

typesanitizer · 2017-04-10T15:34:11Z

@brson , @jonhoo I'm not fully sure what I'm supposed to do here... do I (locally) pull the latest version of master, then rebase it on top of the current branch (fix-issue-954) and then push fix-issue-954 to my Github repo?

Diggsey · 2017-04-10T16:32:04Z

@theindigamer github has instructions for resolving merge conflicts - at the bottom of this PR, there's a button to resolve conflicts online using the web interface, or a link to instructions for doing it via the command line.

typesanitizer · 2017-04-10T16:57:36Z

@Diggsey , thanks. I didn't realize I could edit the text file on the merge conflicts page.

typesanitizer · 2017-04-11T15:04:04Z

If I understand it correctly, the tests are failing because of two reasons:

Some tests like custom_invalid_names and custom_invalid_names_with_archive_dates expect an error based on invalid custom name. Since I added the path checking to the very beginning, the error thrown is that directories are absent, which is unexpected for the test. This can be remedied by moving the directory error-checking to the end of install_from_dir (instead of the beginning).
Some tests like rustup_failed_path_search, custom_toolchain_cargo_fallback_proxy and custom_toolchain_cargo_fallback_run expect toolchain link to succeed silently even if the directory structure is wrong and expect other things to break later. I'm not sure what would be the best way to fix this. Should I just create fake directories and files for bin, lib and rustc?

Should I add a test which ought to fail with the wrong directory structure?

typesanitizer · 2017-04-24T10:43:14Z

@brson , good to merge?

* Rearranges error-checking in `install_from_dir` to pass the tests `custom_invalid_names` and `custom_invalid_names_with_archive_dates`. * Adds subdirectory `lib` to custom directories to pass the tests `custom_toolchain_cargo_fallback_proxy` and `custom_toolchain_cargo_fallback_run`. * Changed `emptydir` to `customdir.join("custom-1") to avoid directory structure error and pass `rustup_failed_search_path`.

brson · 2017-05-08T23:06:12Z

@bors r+ p=1

Yes @theindigamer looks great. Sorry for the long delay.

bors · 2017-05-08T23:06:13Z

📌 Commit 97bee56 has been approved by brson

alexcrichton · 2017-05-09T15:36:59Z

@bors: r=brson

bors · 2017-05-09T15:37:00Z

📌 Commit bd7c369 has been approved by brson

bors · 2017-05-09T17:05:04Z

⌛ Testing commit bd7c369 with merge d330705...

Added help for `rustup toolchain link` as documented in issue #954. * Also renamed TOOLCHAIN_INSTALL_HELP -> INSTALL_HELP for consistency.

bors · 2017-05-09T20:28:30Z

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

Added help for rustup toolchain link as documented in issue rust-la…

ffc4738

…ng#954. * Also renamed TOOLCHAIN_INSTALL_HELP -> INSTALL_HELP for consistency.

typesanitizer mentioned this pull request Mar 29, 2017

Document the use of toolchain link #954

Closed

jonhoo reviewed Mar 29, 2017

View reviewed changes

brson reviewed Mar 31, 2017

View reviewed changes

brson closed this Mar 31, 2017

brson reopened this Mar 31, 2017

Better help and basic error checking for rustup toolchain link (rus…

27202ac

…t-lang#954).

brson reviewed Apr 5, 2017

View reviewed changes

Moves error-checking from toolchain_link into install_from_dir.

bdc7c3a

typesanitizer force-pushed the fix-issue-954 branch from 51e9cff to bdc7c3a Compare April 5, 2017 13:10

Merge branch 'master' into fix-issue-954

c28b805

typesanitizer force-pushed the fix-issue-954 branch from 0d23afb to 97bee56 Compare April 24, 2017 04:23

typesanitizer force-pushed the fix-issue-954 branch from 97bee56 to b459685 Compare April 26, 2017 17:42

typesanitizer force-pushed the fix-issue-954 branch from b459685 to bd7c369 Compare April 26, 2017 17:47

bors added a commit that referenced this pull request May 9, 2017

Auto merge of #1017 - theindigamer:fix-issue-954, r=brson

d330705

Added help for `rustup toolchain link` as documented in issue #954. * Also renamed TOOLCHAIN_INSTALL_HELP -> INSTALL_HELP for consistency.

bors merged commit bd7c369 into rust-lang:master May 9, 2017

typesanitizer deleted the fix-issue-954 branch July 4, 2017 15:33

Conversation

typesanitizer commented Mar 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

typesanitizer Mar 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

typesanitizer Mar 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonhoo Mar 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

typesanitizer Mar 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brson commented Mar 31, 2017

Uh oh!

typesanitizer commented Mar 31, 2017

Uh oh!

jonhoo commented Mar 31, 2017

Uh oh!

typesanitizer commented Mar 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonhoo commented Mar 31, 2017

Uh oh!

typesanitizer commented Mar 31, 2017

Uh oh!

jonhoo commented Mar 31, 2017

Uh oh!

typesanitizer commented Mar 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bors commented Apr 2, 2017

Uh oh!

jonhoo commented Apr 3, 2017

Uh oh!

typesanitizer commented Apr 3, 2017

Uh oh!

jonhoo commented Apr 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brson commented Apr 5, 2017

Uh oh!

brson commented Apr 8, 2017

Uh oh!

jonhoo commented Apr 8, 2017

Uh oh!

brson commented Apr 8, 2017

Uh oh!

typesanitizer commented Apr 10, 2017

Uh oh!

Diggsey commented Apr 10, 2017

Uh oh!

typesanitizer commented Apr 10, 2017

Uh oh!

typesanitizer commented Apr 11, 2017

Uh oh!

typesanitizer commented Apr 24, 2017

Uh oh!

brson commented May 8, 2017

typesanitizer Mar 29, 2017 •

edited

Loading

typesanitizer Mar 29, 2017 •

edited

Loading

jonhoo Mar 29, 2017 •

edited

Loading

typesanitizer Mar 29, 2017 •

edited

Loading

typesanitizer commented Mar 31, 2017 •

edited

Loading

typesanitizer commented Mar 31, 2017 •

edited

Loading