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

Implement shell PATH fixes via UnixShell trait #2387

Merged
merged 30 commits into from
Aug 8, 2020

Conversation

workingjubilee
Copy link
Member

@workingjubilee workingjubilee commented Jun 25, 2020

Resolves #1881, and by doing so also:
Fixes #2106.
Fixes #883.
Fixes #371.
Fixes #332.

This PR significantly changes Rustup's PATH modification strategy:

  1. In $CARGO_HOME an env shell script is written, and now it is sourced in every single rcfile that Rustup has decided to update.
  2. This env shell script describes how to update PATH idempotently.
  3. A new UnixShell trait is added that describes what to write and where for every shell it is implemented for.
  4. It also burns down a lot of outdated code that doesn't rely on this code architecture.

The purpose is to hopefully make it easier to implement new shells in the future and also to fix various incompatibility issues that have historically occurred, especially with regards to GUI terminal emulators.

@workingjubilee workingjubilee force-pushed the impl-shell branch 2 times, most recently from 041e67b to 47ddc21 Compare June 25, 2020 13:09
@workingjubilee
Copy link
Member Author

workingjubilee commented Jun 25, 2020

Alright, further cleanup will be incoming as there's still duplicate code in this rough draft (against the previous draft), but this is and will remain mergeable. It took a While to figure out how best to do it because this is going to wind up cutting out a lot of other code (and I found a surprising number of repeated code, even essentially right after itself), so I wound up trashing a few drafts for maintenance concerns. Got to the point where I wanted to just make sure this got up and passing Windows tests, and in front of other eyes.

The main key here is that the UnixShell trait should localize all shell support implementation so that it only happens in one place from now on, so hopefully when a shell support issue gets raised it does not take one aeon.

@workingjubilee workingjubilee changed the title Implement shell fixes via UnixShell trait Implement shell PATH fixes via UnixShell trait Jun 26, 2020
src/cli/self_update/shell.rs Outdated Show resolved Hide resolved
tests/cli-self-upd.rs Outdated Show resolved Hide resolved
tests/cli-self-upd.rs Outdated Show resolved Hide resolved
@workingjubilee workingjubilee force-pushed the impl-shell branch 2 times, most recently from f612abb to f7663c9 Compare June 27, 2020 00:11
@workingjubilee workingjubilee marked this pull request as draft June 29, 2020 23:56
@workingjubilee workingjubilee marked this pull request as ready for review July 3, 2020 05:03
@workingjubilee workingjubilee requested a review from rbtcollins July 3, 2020 05:04
//! 1) using a shell script that updates PATH if the path is not in PATH
//! 2) sourcing this script in any known and appropriate rc file

use super::canonical_cargo_home;
Copy link
Contributor

Choose a reason for hiding this comment

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

canonical_cargo_home is a bit of an odd function - it transforms any non-clean unicode characters in a bid to make the path printable. This matters when e.g. folk have paths on disk that are in SHIFT-JIS but their encoding is in CP1252 or other incompatible combinations.

So rather than using this function designed for display, call cargo_home()?.join("bin").to_str(). Factoring out a helper for that is just about a common enough thing to be worth doing, but I haven't gotten around to it yet.

If to_str() fails, then we should fail to write the env file for now (as it would be written with bogus content). A more sophisticated iteration of this work would be to write the env file as a bytestring, not unicode, so that we can write it even if cargo_home isn't valid unicode. I don't know if you want to tackle that right now or not.

Copy link
Member Author

Choose a reason for hiding this comment

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

The function should Err in that case, right? What Err should it return? I'm struggling a bit to interpret how to make err_chain work here, looking at it. to_str() gives an Option which I can .ok_or() on.

This dilemma crops up on either path. Frankly it seems like I'm going to go to the same amount of trouble either way so I might as well write some raw bytes, but we'll see.

Copy link
Contributor

@rbtcollins rbtcollins Jul 12, 2020

Choose a reason for hiding this comment

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

if you choose to defer the work and return an error, you'd say something like

let as_str = if let Some(s) = p.to_str() { s } else { bail!("non-unicode homedir") } 

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, that is simpler than expected then, so I will do that and come back to write the bytestring.

Copy link
Member Author

Choose a reason for hiding this comment

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

A later patch, however.

I should note that Rustup was previously using the [lossy canonical_cargo_home]

pub fn shell_export_string() -> Result<String> {
let path = format!("{}/bin", canonical_cargo_home()?);
// The path is *prepended* in case there are system-installed
// rustc's that need to be overridden.
Ok(format!(r#"export PATH="{}:$PATH""#, path))
}
) in this segment of the code anyways so this is already doing additional fixes.


// Gives all rcfiles of a given shell that rustup is concerned with.
// Used primarily in checking rcfiles for cleanup.
fn rcfiles(&self) -> Vec<PathBuf>;
Copy link
Contributor

Choose a reason for hiding this comment

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

See below - but I think rcfiles vs update_rcs is an unneeded distinction, because we can be pretty certain we won't be pure as far as updating bash's profile files is concerned, because of the intersection with the posix shell definition.

Copy link
Member Author

Choose a reason for hiding this comment

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

Mm, the evolution of the zsh implementation suggests it is going to be a concern anyways.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking through this, I cannot see rcfiles() being called on the trait; only from within trait implementations. All I'm suggesting is that it be removed from the trait unless-and-until something that is calling methods on the trait needs to call it.

tests/cli-self-upd.rs Outdated Show resolved Hide resolved
@rbtcollins
Copy link
Contributor

I think we also need a genuinely new test: that upgrading rustup removes the old added PATH mechanism at the same time it installs the new PATH env mechanism.

@workingjubilee
Copy link
Member Author

Yes, test was added, should be functional!

Copy link
Contributor

@rbtcollins rbtcollins left a comment

Choose a reason for hiding this comment

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

I think the code itself is basically sound and we're in fine tuning over some test code rearrangements.

src/cli/self_update.rs Outdated Show resolved Hide resolved
src/cli/self_update/shell.rs Show resolved Hide resolved
src/cli/self_update/shell.rs Outdated Show resolved Hide resolved
src/cli/self_update/shell.rs Show resolved Hide resolved
src/cli/self_update/shell.rs Outdated Show resolved Hide resolved
tests/cli-paths.rs Outdated Show resolved Hide resolved
tests/cli-paths.rs Outdated Show resolved Hide resolved
tests/cli-paths.rs Outdated Show resolved Hide resolved
tests/cli-paths.rs Show resolved Hide resolved
tests/cli-self-upd.rs Outdated Show resolved Hide resolved
@workingjubilee workingjubilee force-pushed the impl-shell branch 2 times, most recently from 014900f to fa10206 Compare July 18, 2020 01:31
A lot of the tests moved to cli-paths used mutexes or init commands
that performed a lot of unnecessary behavior, especially on Unix.
By defaulting to no toolchain and removing the mutex this can be
sped up quite a bit.
tests/cli-paths.rs Outdated Show resolved Hide resolved
@workingjubilee
Copy link
Member Author

Linux error is spurious (on snapcraft).

src/cli/self_update.rs Outdated Show resolved Hide resolved
src/cli/self_update.rs Outdated Show resolved Hide resolved
src/cli/self_update/shell.rs Show resolved Hide resolved
src/cli/self_update/shell.rs Outdated Show resolved Hide resolved
tests/cli-self-upd.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@kinnison kinnison left a comment

Choose a reason for hiding this comment

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

I think this is close to ready. @rbtcollins Could you go through your outstanding comments and resolve/follow up if needed?

Let's see if we can get this through the door.

src/cli/self_update/shell.rs Show resolved Hide resolved
Copy link
Contributor

@rbtcollins rbtcollins left a comment

Choose a reason for hiding this comment

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

Sorry about the long delay; busy in life.

I think this is basically busy, the only thing remaining is the question I raised earlier about the unused method on the trait. I'm ok with leaving it as-is, but there really doesn't seem to be any use for it.


// Gives all rcfiles of a given shell that rustup is concerned with.
// Used primarily in checking rcfiles for cleanup.
fn rcfiles(&self) -> Vec<PathBuf>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking through this, I cannot see rcfiles() being called on the trait; only from within trait implementations. All I'm suggesting is that it be removed from the trait unless-and-until something that is calling methods on the trait needs to call it.

fn update_rcs(&self) -> Vec<PathBuf> {
// Write to .profile even if it doesn't exist. It's the only rc in the
// POSIX spec so it should always be set up.
self.rcfiles()
Copy link
Contributor

Choose a reason for hiding this comment

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

With the suggestion above, rcfiles here could be inlined into update_rcs for this impl.

Copy link
Member Author

Choose a reason for hiding this comment

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

unix.sh, line 65:

        for rc in sh.rcfiles().iter().filter(|rc| rc.is_file()) {

Copy link
Member Author

Choose a reason for hiding this comment

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

I know it's basically that one place. ^^;
But it is a method that has a reason for existing.

src/cli/self_update/shell.rs Show resolved Hide resolved
src/cli/self_update/shell.rs Show resolved Hide resolved
@workingjubilee
Copy link
Member Author

workingjubilee commented Aug 7, 2020

We discussed and there was an understanding gap that has been bridged, in terms of smoothing out the code. The specific requests were in service of a goal I did not understand (and initially did not inquire too deeply about). As I now understand the higher-level goal (difficult-to-itemize improvements in clarity), I can draft some commits in service of that.

That is the only objection standing, however, and I will note that it is the only issue, and should not result in major logical changes, so it could be merged as a later PR. However, as long as this one remains open, I will work on that issue here.

@rbtcollins rbtcollins merged commit 0db31cd into rust-lang:master Aug 8, 2020
@rbtcollins
Copy link
Contributor

Please do continue on and do the improvements we've discussed, but per discord, there's a bias toward getting this in at this stage, it is good enough - so I've merged it.

@workingjubilee
Copy link
Member Author

Ah good. That makes patching this work from multiple directions actually easier for me since it's not all one big diff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants