-
Notifications
You must be signed in to change notification settings - Fork 255
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
feat: support yarnpkg as an alias for yarn #1670
Conversation
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.
This seems totally on the right path! Thanks for working on it!!
I think there are a few things we'd still need to do:
- Ensure we create the
yarnpkg
shim (will require changes in the installer I think?)
Line 94 in 889ff97
ln -s "${INSTALL_DIR}"/shim "${INSTALL_DIR}"/bin/yarn |
volta/dev/unix/volta-install-legacy.sh
Line 77 in 889ff97
local main_shims=( node npm npx yarn ) |
Lines 157 to 172 in 889ff97
<Component Id='yarnBinary' Guid='*' Win64='$(var.Win64)'> | |
<File | |
Id='yarnEXE' | |
Name='yarn.exe' | |
DiskId='1' | |
Source='target\release\volta-shim.exe' | |
KeyPath='yes'/> | |
</Component> | |
<Component Id='yarnScript' Guid='*' Win64='$(var.Win64)'> | |
<File | |
Id='yarnCMD' | |
Name='yarn.cmd' | |
DiskId='1' | |
Source='wix\shim.cmd' | |
KeyPath='yes'/> | |
</Component> |
- Add some tests so we don't accidentally break this new feature. I'm not 100% sure where to add the tests. Maybe @chriskrycho or @charlespierce have an idea...
Cargo.toml
Outdated
@@ -1,6 +1,6 @@ | |||
[package] | |||
name = "volta" | |||
version = "1.1.1" | |||
version = "1.1.2" |
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 general, we only bump the version when we do releases.
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.
Also need to make sure volta run
knows how to handle this correctly. I think that means adding another match arm here:
volta/crates/volta-core/src/run/mod.rs
Line 99 in 889ff97
Some("yarn") => yarn::command(args, session), |
Some tests are here (you could probably add a couple that call volta run --yarn=1.7.71 yarnpkg --version
or something):
volta/tests/acceptance/volta_run.rs
Lines 357 to 433 in 889ff97
#[test] | |
fn command_line_yarn_1() { | |
let s = sandbox() | |
.node_available_versions(NODE_VERSION_INFO) | |
.distro_mocks::<NodeFixture>(&NODE_VERSION_FIXTURES) | |
.yarn_1_available_versions(YARN_1_VERSION_INFO) | |
.distro_mocks::<Yarn1Fixture>(&YARN_1_VERSION_FIXTURES) | |
.env(VOLTA_LOGLEVEL, "debug") | |
.build(); | |
assert_that!( | |
s.volta("run --node 10.99.1040 --yarn 1.7.71 yarn --version"), | |
execs() | |
.with_status(ExitCode::Success as i32) | |
.with_stderr_contains("[..]Yarn: 1.7.71 from command-line configuration") | |
); | |
} | |
#[test] | |
fn command_line_yarn_3() { | |
let s = sandbox() | |
.node_available_versions(NODE_VERSION_INFO) | |
.distro_mocks::<NodeFixture>(&NODE_VERSION_FIXTURES) | |
.yarn_1_available_versions(YARN_1_VERSION_INFO) | |
.yarn_berry_available_versions(YARN_BERRY_VERSION_INFO) | |
.distro_mocks::<Yarn1Fixture>(&YARN_1_VERSION_FIXTURES) | |
.distro_mocks::<YarnBerryFixture>(&YARN_BERRY_VERSION_FIXTURES) | |
.env(VOLTA_LOGLEVEL, "debug") | |
.build(); | |
assert_that!( | |
s.volta("run --node 10.99.1040 --yarn 3.7.71 yarn --version"), | |
execs() | |
.with_status(ExitCode::Success as i32) | |
.with_stderr_contains("[..]Yarn: 3.7.71 from command-line configuration") | |
); | |
} | |
#[test] | |
fn inherited_yarn_1() { | |
let s = sandbox() | |
.node_available_versions(NODE_VERSION_INFO) | |
.distro_mocks::<NodeFixture>(&NODE_VERSION_FIXTURES) | |
.yarn_1_available_versions(YARN_1_VERSION_INFO) | |
.distro_mocks::<Yarn1Fixture>(&YARN_1_VERSION_FIXTURES) | |
.package_json(&package_json_with_pinned_node_yarn("10.99.1040", "1.2.42")) | |
.env(VOLTA_LOGLEVEL, "debug") | |
.build(); | |
assert_that!( | |
s.volta("run --node 10.99.1040 yarn --version"), | |
execs() | |
.with_status(ExitCode::Success as i32) | |
.with_stderr_contains("[..]Yarn: 1.2.42 from project configuration") | |
); | |
} | |
#[test] | |
fn inherited_yarn_3() { | |
let s = sandbox() | |
.node_available_versions(NODE_VERSION_INFO) | |
.distro_mocks::<NodeFixture>(&NODE_VERSION_FIXTURES) | |
.yarn_1_available_versions(YARN_1_VERSION_INFO) | |
.yarn_berry_available_versions(YARN_BERRY_VERSION_INFO) | |
.distro_mocks::<Yarn1Fixture>(&YARN_1_VERSION_FIXTURES) | |
.distro_mocks::<YarnBerryFixture>(&YARN_BERRY_VERSION_FIXTURES) | |
.package_json(&package_json_with_pinned_node_yarn("10.99.1040", "3.2.42")) | |
.env(VOLTA_LOGLEVEL, "debug") | |
.build(); | |
assert_that!( | |
s.volta("run --node 10.99.1040 yarn --version"), | |
execs() | |
.with_status(ExitCode::Success as i32) | |
.with_stderr_contains("[..]Yarn: 3.2.42 from project configuration") | |
); | |
} |
Interesting! Something gave me a yarnpkg symlink under .volta/bin--but I'll take a look at your links a bit later and see if I can figure what's what. |
Most likely that was done by |
Did I mess something up in the PR? I thought I handled it here: volta/crates/volta-core/src/run/mod.rs Lines 99 to 100 in 994925b
And here: volta/crates/volta-core/src/run/yarn.rs Line 22 in 994925b
volta/crates/volta-core/src/run/yarn.rs Line 40 in 994925b
As for the tests--do you prefer a separate test that duplicates the setup, or would a second assertion in the test do? And finally: yarn 3 doesn't seem to ship the yarnpkg command. That results in a NoPlatform error, which is reported as a missing node installation. The NoPlatform error should probably include the name of the missing component? Should I try to handle the yarn 3 case gracefully, or would the error suffice, if the message was correct? |
Nope, you didn't miss anything. I did! 😸 I was jumping between main and your PR, and must have gotten confused about which branch I was on. 🤦
I think additional assertions are fine (though I'll defer to @chriskrycho / @charlespierce if they have a strong preference).
Oof. That makes things a bit rough. I do worry about the situation where someone might check (e.g. in a script do
Yeah, I think we'll need to tweak this. I'd prefer to have a custom error specifically for this scenario (usage of |
Regarding the Yarn 3 / That would mean that |
That makes more sense than any other alternative I can think of, yes. I reverted the yarn::command change and combined the yarn and yarnpkg arms of the match into one. |
Anything else I should do to get this finalized? I just verified that this works on Windows--I went down this rabbit hole because I noticed that the lack of a |
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.
Seems good to me!
@charlespierce / @chriskrycho - Any other changes needed from y'alls perspective?
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.
Nope, seems excellent, let's do it! (And sorry for the delay!)
My attempt at implementing #1391. This is my first Rust code aside from a helloworld example, so you know, it mightn't be perfect yet.
I tested (and wrote) this on macOS Sonoma 14.3 by switching between the 1.1.1 release and this dev build, and it seems to work, but I'm not 100% sure on how to reliably verify it. I bumped the version to 1.1.2 so that the dev install script would let me switch, not sure if I was supposed to do that. :)