-
Notifications
You must be signed in to change notification settings - Fork 877
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
Added help for rustup toolchain link
as documented in issue #954.
#1017
Conversation
typesanitizer
commented
Mar 29, 2017
- Also renamed TOOLCHAIN_INSTALL_HELP -> INSTALL_HELP for consistency.
…ng#954. * Also renamed TOOLCHAIN_INSTALL_HELP -> INSTALL_HELP for consistency.
src/rustup-cli/help.rs
Outdated
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 asstable
,nightly
,1.8.0
, etc., thenpath
will be used in place of the standard binaries for those toolchains. For example, if a toolchain symlink is set up callednightly
, any directory with anightly
override will now also use the new symlink.
Also, I notice the text currently says
For more information see
rustup help toolchain
Doesrustup help toolchain
actually give more information at the moment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 install
so I thought I should follow suit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 therustup toolchain link
command. This can
also be used to symlink toolchains from local builds. For more
information seerustup 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.
src/rustup-cli/help.rs
Outdated
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`."; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
src/rustup-cli/help.rs
Outdated
|
||
'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'. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ...".
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. |
@brson , @jonhoo
That said, here are all the changes I propose:
(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 |
@theindigamer good catch! The updated text looks pretty good, though I'd suggest two changes:
|
Alright. Updated text for
EDITS:
|
Any particular reason you want to set the new one as |
@jonhoo , good point. I didn't think about the |
I think that means you can also remove " with default settings". With that change I'm happy :) |
I have the bit about default settings because someone might have declared an explicit dependency on a 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 |
☔ The latest upstream changes (presumably #1019) made this pull request unmergeable. Please resolve the merge conflicts. |
@theindigamer the latest draft of the text looks good. I see you also pushed some checks for the existence of |
This is a minimal check. There may be additional folders such as
The |
Okay, you've got my r , even though you don't need it :p |
src/rustup-cli/rustup_mode.rs
Outdated
pathbuf.push("bin"); | ||
try!(utils::assert_is_directory(&pathbuf)); | ||
pathbuf.push("rustc"); | ||
try!(utils::assert_is_file(&pathbuf)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
This looks great to me. I just added one nit about where to do the new validation. Thanks @theindigamer and @jonhoo. |
51e9cff
to
bdc7c3a
Compare
Looks like there's a merge conflict now @jonhoo. Can you rebase? |
@theindigamer owns this, so he has to be the one to do it :) |
Lol sorry @jonhoo . |
@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. |
@Diggsey , thanks. I didn't realize I could edit the text file on the merge conflicts page. |
If I understand it correctly, the tests are failing because of two reasons:
Should I add a test which ought to fail with the wrong directory structure? |
0d23afb
to
97bee56
Compare
@brson , good to merge? |
97bee56
to
b459685
Compare
* 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`.
b459685
to
bd7c369
Compare
@bors r p=1 Yes @theindigamer looks great. Sorry for the long delay. |
📌 Commit 97bee56 has been approved by |
@bors: r=brson |
📌 Commit bd7c369 has been approved by |
Added help for `rustup toolchain link` as documented in issue #954. * Also renamed TOOLCHAIN_INSTALL_HELP -> INSTALL_HELP for consistency.
☀️ Test successful - status-appveyor, status-travis |