Check volta shims for reachability during tool install #1292

untitaker · 2022-08-12T19:54:38Z

charlespierce

Thanks for this @untitaker, and apologies for the delay in reviewing! I've unfortunately been increasingly busy with my day job and haven't had the time to dedicate to keeping up with reviews.

Left a couple of suggestions for code organization, but the overall approach is sound and I think will be a big help 🎉

charlespierce · 2022-09-16T21:32:45Z

crates/volta-core/src/shim.rs

@@ -89,6 +89,23 @@ pub fn create(shim_name: &str) -> Fallible<ShimResult> {
    }
 }

+pub fn check_reachable(shim_name: &str) -> Fallible<()> {


Rather than make the public method fallible (and need to create a new error type), since this is intended to be purely informational, can we make this infallible and only show a message if we're able to successfully determine both the shim and the resolved and compare them?

Separately, also a slight preference for moving this into the tool/mod.rs file, rather than holding it here in shim.rs. The main reason for that is we can then use the tool helper functions for writing the message out (e.g. note_prefix() and similar that are used for other notes).

Alternatively, we could have this method be something like is_shadowed(shim_name: &str) -> ShadowStatus {} with an enum:

pub enum ShadowStatus { NotShadowed, Shadowed { expected: PathBuf, found: PathBuf, } }

And then only build the message inside of the tool module.

charlespierce · 2022-09-16T21:40:26Z

crates/volta-core/src/shim.rs

+        )
+    })?;
+    if resolved != shim {
+        warn!("{} is shadowed by another binary of the same name at {}. Please ensure that {} is at the front of your PATH.", shim_name, resolved.display(), shim.display());


Given where this is used, rather than warn!, I would lean a little towards using info! and note_prefix, as that's the approach we've taken for a similar message when you install a global version of Node but the local version is different. In both cases, the message exists to let you know that while you just installed something, what you get when you run the command may not be what you expect.

charlespierce · 2022-09-16T21:41:15Z

crates/volta-core/src/error/kind.rs

@@ -429,6 +429,11 @@ pub enum ErrorKind {
        name: String,
    },

+    /// Thrown when Volta is unable to find the shimmed command.
+    ShimResolveError {


As mentioned below, if we make the lookup infallible by handling (i.e. ignoring) the error directly, we shouldn't need a new error type.

charlespierce · 2022-09-16T21:45:33Z

tests/acceptance/support/sandbox.rs

+    /// Add an arbitrary file to the test project within the sandbox,
+    /// give it executable permissions,
+    /// and add its directory to the *front* of PATH, shadowing any volta binaries.


nit: Can we update this doc comment to more accurately reflect what the method does? It appears that it doesn't work with any files, rather it only does the prepending. Can we also document the constraint that prepend_exec_dir_to_path is exclusive of add_exec_dir_to_path?

felipecrs · 2022-10-03T14:44:22Z

I think this also fixes #1053.

untitaker · 2022-10-20T10:51:55Z

@charlespierce i think i addressed all comments. initially i thought it would make sense to make volta hard-crash with exitcode 1 if the PATH can't be resolved to the shim (i think that would be useful when volta is used in CI), but when going back to this code i saw that only some error conditions were treated as fatal.

charlespierce

Thanks for continuing to iterate on this! A couple of small suggested tweaks to the language, to make sure that we are providing the best UX that we can in these situations.

There also appears to be a merge conflict with dependency updates.

charlespierce · 2022-11-01T18:49:41Z

crates/volta-core/src/tool/mod.rs

+        Ok(resolved) => resolved,
+        Err(_) => {
+            info!(
+                "{} cannot find command {}. Please ensure that {} is at the front of your PATH.",


Apologies for multiple rounds of iteration, but for an error / info message, we try to take special care to be as helpful as possible. Three relatively minor things that jump out here:

Would it make sense to use the home.shim_dir() value for the CTA part? Using the shim path itself would mean that we would suggest putting ~/.volta/bin/node on the PATH, when what we're actually suggesting is having ~/.volta/bin on the PATH

Since Volta works on Windows as well (and we do these same checks), we should match Windows' casing for Path on those systems. So we should have PATH on Unix and Path on Windows.

Also related to Windows, with how the Environment variables are handled, I'm not sure "at the front of your Path" makes the most sense. For this case (the command couldn't be found at all), we can probably say something like "Please ensure that {} is available on your Path".

charlespierce · 2022-11-01T18:53:25Z

crates/volta-core/src/tool/mod.rs

+        }
+    };
+    if resolved != shim {
+        info!("{} {} is shadowed by another binary of the same name at {}. Please ensure that {} is at the front of your PATH.", note_prefix(), shim_name, resolved.display(), shim.display());


Similar comments as above for this message re shim_dir and PATH/Path. I also wonder if it would make sense to expand the CTA somewhat, to make it clear why we're suggesting it? Perhaps something like:

To ensure your commands work as expected, please move {} to the start of your PATH.

untitaker · 2022-11-08T13:45:54Z

@charlespierce I think I fixed both comments

untitaker · 2023-03-09T19:16:01Z

@charlespierce could you take a look?

chriskrycho · 2023-12-19T22:17:12Z

Sorry about the delay here, @untitaker – will see if we can get this reviewed and merged here shortly (at the start of the new year, if nothing else)!

charlespierce

LGTM! 🎉

chriskrycho · 2024-01-18T20:53:51Z

I'll merge in main to make sure CI is still ✅ and we'll merge it! 🎉

chriskrycho · 2024-01-18T21:55:43Z

Ah, I messed up the merge commit; I will re-create it and force push the correct version in a bit!

Using `#[cfg(...)]` attributes on expressions is unstable, so switch to defining a constant with the relevant value behind a `cfg_if!()`, as we do elsewhere.

untitaker · 2024-01-19T00:06:10Z

I can try to fix the test

chriskrycho · 2024-01-19T00:07:28Z

@untitaker that would be great! I fixed up the one build issue I saw, so you should be unblocked. Let us know when it's fixed and we'll get this merged. Thanks again, and thanks for your patience!

untitaker · 2024-01-19T00:14:58Z

Running this locally:

RUST_BACKTRACE=1 cargo test --all --features mock-network

gives me this output for most tests:


thread 'volta_run::inherited_pnpm' panicked at /Users/untitaker/.cargo/registry/src/index.crates.io-6f17d22bba15001f/mockito-0.31.1/src/lib.rs:1184:72:
called `Result::unwrap()` on an `Err` value: Os { code: 2, kind: NotFound, message: "No such file or directory" }
stack backtrace:
   0: rust_begin_unwind
             at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/std/src/panicking.rs:645:5
   1: core::panicking::panic_fmt
             at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/core/src/panicking.rs:72:14
   2: core::result::unwrap_failed
             at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/core/src/result.rs:1653:5
   3: core::result::Result<T,E>::unwrap
             at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/core/src/result.rs:1077:23
   4: mockito::Mock::with_body_from_file
             at /Users/untitaker/.cargo/registry/src/index.crates.io-6f17d22bba15001f/mockito-0.31.1/src/lib.rs:1184:52
   5: acceptance::support::sandbox::SandboxBuilder::distro_mock
             at ./tests/acceptance/support/sandbox.rs:457:25
   6: acceptance::support::sandbox::SandboxBuilder::distro_mocks
             at ./tests/acceptance/support/sandbox.rs:470:20
   7: acceptance::volta_run::inherited_pnpm
             at ./tests/acceptance/volta_run.rs:475:13
   8: acceptance::volta_run::inherited_pnpm::{{closure}}
             at ./tests/acceptance/volta_run.rs:474:20
   9: core::ops::function::FnOnce::call_once
             at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/core/src/ops/function.rs:250:5
  10: core::ops::function::FnOnce::call_once
             at /rustc/82e1608dfa6e0b5569232559e3d385fea5a93112/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

untitaker · 2024-01-25T13:32:23Z

@chriskrycho can you or somebody else help me figure out what is broken with my devenv? I can't figure out the broken tests before that. I am following the instructions in CONTRIBUTING

chriskrycho · 2024-01-26T04:10:47Z

Huh. That should have been fixed by some of the work @rwjblue and I did a little while back, but it's possible that we missed something there or that I did the merge above wrong or that we caused a regression. The issue is that the test mock is asking for a path named "tests/fixtures/node-v10.99.1040-darwin-arm64.tar.gz" (and so on for the other failing tests), and there is no darwin-arm64 fixture for Node v10.99, because that would be a nonsense thing to ask for. What I am trying to figure out now is whether that error came from, because it should certainly be entirely unrelated to your changes here! My current best bet is that I messed up merging into your branch somehow!

untitaker · 2024-02-17T02:46:44Z

there is no darwin-arm64 fixture for Node v10.99, because that would be a nonsense thing to ask for.

darwin-arm64 does make sense to me, that's just any new MacBook. So I suppose the devenv does not work on any M1 or M2 macbook? Not sure

I switched to an x86 laptop for this PR, and everything worked out. Fixed the remaining test failure, so I think this PR should be ready to merge.

Check volta shims for reachability during tool install

ca046de

Fix volta-cli#1268

charlespierce requested changes Sep 16, 2022

View reviewed changes

apply review feedback

a201041

add missing note prefix

2bb4c25

charlespierce requested changes Nov 1, 2022

View reviewed changes

untitaker added 2 commits November 7, 2022 15:54

apply review feedback

bdcf933

Merge remote-tracking branch 'origin/main' into sanity-check-shims

0148a0b

Merge remote-tracking branch 'origin/main' into sanity-check-shims

1679c39

untitaker requested a review from charlespierce January 30, 2023 18:40

charlespierce approved these changes Jan 18, 2024

View reviewed changes

chriskrycho added the bug label Jan 18, 2024

chriskrycho force-pushed the sanity-check-shims branch from 67ce551 to 1679c39 Compare January 18, 2024 23:47

chriskrycho added 2 commits January 18, 2024 16:48

Merge remote-tracking branch 'origin/main' into sanity-check-shims

a3b2c57

Define path environment variable with a const

301a162

Using `#[cfg(...)]` attributes on expressions is unstable, so switch to defining a constant with the relevant value behind a `cfg_if!()`, as we do elsewhere.

its stdout now i guess

5b820a3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check volta shims for reachability during tool install #1292

Check volta shims for reachability during tool install #1292

untitaker commented Aug 12, 2022

charlespierce left a comment

charlespierce Sep 16, 2022

charlespierce Sep 16, 2022

charlespierce Sep 16, 2022

charlespierce Sep 16, 2022

charlespierce Sep 16, 2022

felipecrs commented Oct 3, 2022

untitaker commented Oct 20, 2022

charlespierce left a comment

charlespierce Nov 1, 2022

charlespierce Nov 1, 2022

untitaker commented Nov 8, 2022

untitaker commented Mar 9, 2023

chriskrycho commented Dec 19, 2023

charlespierce left a comment

chriskrycho commented Jan 18, 2024

chriskrycho commented Jan 18, 2024

untitaker commented Jan 19, 2024

chriskrycho commented Jan 19, 2024

untitaker commented Jan 19, 2024

untitaker commented Jan 25, 2024

chriskrycho commented Jan 26, 2024

untitaker commented Feb 17, 2024 •

edited

Check volta shims for reachability during tool install #1292

Are you sure you want to change the base?

Check volta shims for reachability during tool install #1292

Conversation

untitaker commented Aug 12, 2022

charlespierce left a comment

Choose a reason for hiding this comment

charlespierce Sep 16, 2022

Choose a reason for hiding this comment

charlespierce Sep 16, 2022

Choose a reason for hiding this comment

charlespierce Sep 16, 2022

Choose a reason for hiding this comment

charlespierce Sep 16, 2022

Choose a reason for hiding this comment

charlespierce Sep 16, 2022

Choose a reason for hiding this comment

felipecrs commented Oct 3, 2022

untitaker commented Oct 20, 2022

charlespierce left a comment

Choose a reason for hiding this comment

charlespierce Nov 1, 2022

Choose a reason for hiding this comment

charlespierce Nov 1, 2022

Choose a reason for hiding this comment

untitaker commented Nov 8, 2022

untitaker commented Mar 9, 2023

chriskrycho commented Dec 19, 2023

charlespierce left a comment

Choose a reason for hiding this comment

chriskrycho commented Jan 18, 2024

chriskrycho commented Jan 18, 2024

untitaker commented Jan 19, 2024

chriskrycho commented Jan 19, 2024

untitaker commented Jan 19, 2024

untitaker commented Jan 25, 2024

chriskrycho commented Jan 26, 2024

untitaker commented Feb 17, 2024 • edited

untitaker commented Feb 17, 2024 •

edited