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

Inconsistency between tool installation #265

Closed
rotu opened this issue Oct 27, 2023 · 17 comments
Closed

Inconsistency between tool installation #265

rotu opened this issue Oct 27, 2023 · 17 comments
Assignees
Labels
bug Something isn't working

Comments

@rotu
Copy link
Contributor

rotu commented Oct 27, 2023

What version?

0.21.0

Which command?

No response

What happened?

How proto installs and refers to various executables is confusing and inconsistent:

node exists as a bin symlink and a global shims script.
npm exists as a bin symlink but NOT a global shims.

proto bin node and proto bin node --shim both show the versioned node executable .proto\tools\node\20.9.0\node.exe.
proto bin npm shows me the versioned node .proto\tools\npm\10.1.0\bin/npm-cli.js and proto bin npm --shim shows me a powershell script .proto\tools\npm\10.1.0\shims\npm.ps1

proto bin npm shows the path to the current npm.
proto bin npx fails with "npx is not a built-in tool or has not been configured as a plugin, unable to proceed."

Any logs?

No response

Operating system?

Windows

Architecture?

x64

@rotu rotu added the bug Something isn't working label Oct 27, 2023
@milesj
Copy link
Contributor

milesj commented Oct 27, 2023

@rotu I think the confusion lies in the fact that there's 2 kinds of shims. There's the global shim at ~/.proto/shims (formerly ~/.proto/bin) that all tools should have, and then there's a local shim at ~/.proto/tools/<tool>/<version>/shims that only a few tools have.

The local shims are a special case that need unique functionality. Right now its npm/pnpm/yarn because they require node to execute .js files, since you can't execute them directly.

proto bin is for accessing the real binary of the tool, not our symlink or our shim. The --shim arg returns the local shim, because again, some tools original binaries cannot be executed directly. This also returns the real binary if a tool doesn't have a local shim.

@milesj
Copy link
Contributor

milesj commented Oct 27, 2023

npm exists as a bin symlink but NOT a global shims.

This seems wrong though. Are you saying ~/.proto/shims/npm does not exist? Even after reinstalling npm?

@rotu
Copy link
Contributor Author

rotu commented Oct 28, 2023

This seems wrong though. Are you saying ~/.proto/shims/npm does not exist? Even after reinstalling npm?

There's a fair bet I messed that observation up.

Trying it now on a fresh Windows VM with npm and node installed via proto shows a global bin symlink for node and cmd shims for node-gyp, node, npm, npx

I think the confusion lies in the fact that there's 2 kinds of shims

That's definitely part of my confusion - until you mentioned it, I didn't even notice the local shims!

Also, for some reason, on Windows, global shims are only .cmd but local shims are only .ps1?


The below observations are all on my Mac:

proto install node latest -- --no-bundled-npm:
bin: node
shims: node npx

proto install npm:
bin: npm
shims: node-gyp npm

Note this is especially weird since npx is part of npm, not node.


proto bin npm: /Users/dan/.proto/tools/npm/10.1.0/bin/npm-cli.js

proto bin npx: "npx is not a built-in tool or has not been configured as a plugin, unable to proceed."

@milesj
Copy link
Contributor

milesj commented Oct 28, 2023

Also, for some reason, on Windows, global shims are only .cmd but local shims are only .ps1?

.ps1 isn't executable by default on PATH, only .cmd. Users would need to update PATHEXT but that's a hassle. The local shim is .ps1 because it doesn't rely on path.

Note this is especially weird since npx is part of npm, not node.

npx is also published by node. But I agree, this will probably change in the future.

npx is not a built-in tool or has not been configured as a plugin, unable to proceed.

npx isn't a proto tool, so using proto bin npx doesn't make sense. npx is a secondary shim created by a primary tool, since tools can create multiple shims. Same reason why proto bin node-gyp doesn't work.

@rotu
Copy link
Contributor Author

rotu commented Oct 28, 2023

.ps1 isn't executable by default on PATH, only .cmd. Users would need to update PATHEXT but that's a hassle. The local shim is .ps1 because it doesn't rely on path.

I think I'm missing something; I thought the whole reason to have a shim in the first place is to add it to the path.

npx isn't a proto tool, so using proto bin npx doesn't make sense. npx is a secondary shim created by a primary tool, since tools can create multiple shims. Same reason why proto bin node-gyp doesn't work.

I understand there is a technical reason why this works the way it does, but it seems arbitrary from a usage standpoint. E.g. proto run npm works but proto run pip does not.

I would expect proto bin <executable> to tell me about any binary important enough to create a shim for, especially if it's not named the same as the tool providing it!

@milesj
Copy link
Contributor

milesj commented Oct 28, 2023

I think I'm missing something; I thought the whole reason to have a shim in the first place is to add it to the path.

Correct, but .ps1 files aren't executed when on PATH.

I would expect proto bin to tell me about any binary important enough to create a shim for, especially if it's not named the same as the tool providing it!

We don't know what tool owns what shims without actually loading the plugin and executing the tool, which requires knowing the tool name ahead of time.

proto bin was for those rare use cases when you needed to do something like $(proto bin tool) in bash. Once the ~/.proto/bin directory is "complete", we can probably just remove that command.

@rotu
Copy link
Contributor Author

rotu commented Oct 28, 2023

I think I'm missing something; I thought the whole reason to have a shim in the first place is to add it to the path.

Correct, but .ps1 files aren't executed when on PATH.

Okay. So what is this file for? .proto\tools\npm\10.1.0\shims\npm.ps1

I would expect proto bin to tell me about any binary important enough to create a shim for, especially if it's not named the same as the tool providing it!

We don't know what tool owns what shims without actually loading the plugin and executing the tool, which requires knowing the tool name ahead of time.

proto bin was for those rare use cases when you needed to do something like $(proto bin tool) in bash. Once the ~/.proto/bin directory is "complete", we can probably just remove that command.

Gotcha. The original motivation behind this issue was that I was trying to understand what binaries proto has and add a workaround in my Powershell profile to problems that might be incurred by the shim files themselves ("Terminate batch job (Y/N)" showing up, sometimes twice; and Node continuing to run after ctrl-C).

It would be nice in general to have a command for introspecting tools (what shims does this tool provide? Which prototools/package.json/environment variable specified the tool version?)

@milesj
Copy link
Contributor

milesj commented Oct 28, 2023

Okay. So what is this file for? .proto\tools\npm\10.1.0\shims\npm.ps1

I explained it here: #265 (comment) If you compare the local and global shim, they're a bit different.

It would be nice in general to have a command for introspecting tools (what shims does this tool provide? Which prototools/package.json/environment variable specified the tool version?)

Yeah eventually there will be like a proto plugin info <tool> command or something.

@rotu
Copy link
Contributor Author

rotu commented Oct 29, 2023

I explained it here: #265 (comment) If you compare the local and global shim, they're a bit different.

Here's my understanding.

On Windows, the global npm shim is:

@echo off
setlocal

set "ErrorActionPreference=Stop"

if defined PROTO_DEBUG (
    set "DebugPreference=Continue"
)

proto.exe run npm --  %* 

This sets the ErrorActionPreference and DebugPreference environment variables (though I think these are specific to PowerShell and have no effect here?) then calls proto run npm, forwarding arguments.

Then (and this is the part I missed), proto launches a new PowerShell process to run the local shim. (I assumed that the binary proto bin npm returns is what proto run npm runs, and that shims are only for user convenience.)

let mut command = match bin_path.extension().map(|e| e.to_str().unwrap()) {
Some("ps1") => {
let mut cmd = Command::new(if is_command_on_path("pwsh") {
"pwsh"
} else {
"powershell"
});
cmd.arg("-Command").arg(format!(
"{} {}",
bin_path.display(),
args.passthrough.join(" ")
));
cmd
}
Some("cmd" | "bat") => {
let mut cmd = Command::new("cmd");
cmd.arg("/q").arg("/c").arg(format!(
"{} {}",
bin_path.display(),
args.passthrough.join(" ")
));
cmd
}
_ => {
let mut cmd = Command::new(bin_path);
cmd.args(&args.passthrough);
cmd
}
};

The local Windows npm shim is:

#!/usr/bin/env pwsh
$ErrorActionPreference = 'Stop'

if (Test-Path env:PROTO_DEBUG) {
    $DebugPreference = 'Continue'
}

[Environment]::SetEnvironmentVariable('PROTO_NPM_DIR', '/tmp/proto_test\tools\npm\10.2.0', 'Process')
[Environment]::SetEnvironmentVariable('PROTO_NPM_VERSION', '10.2.0', 'Process')

if (Test-Path env:PROTO_NODE_BIN) {
    $parent = $Env:PROTO_NODE_BIN
} else {
    $parent = "node"
}

& "$parent" "/tmp/proto_test\tools\npm\10.2.0\bin/npm-cli.js"  $args 

exit $LASTEXITCODE

This sets the variables $ErrorActionPreference and $DebugPreference (this time, I think they do affect PowerShell).
Then it sets PROTO_NPM_DIR, PROTO_NPM_VERSION, PROTO_NODE_BIN to make sure an inner call to proto run (possibly via the shim) will resolve the npm and node binaries.

Then it calls node to run npm-cli, (but I think it forwards the arguments incorrectly; $args is a string, and that should be @args).


This is pretty hard to follow. My gut is that proto shouldn't be using PowerShell and internal shims like this.

@milesj
Copy link
Contributor

milesj commented Oct 29, 2023

To start, the global shim triggers proto run ... so that the runtime version detection works.

The local shim does not use proto run and instead executes the .js file directly (using node as the parent binary) to avoid the overhead again. While this is true for npm (in this example), the node binary in the global shim may still trigger it's own version detection for Node.js.

Then it calls node to run npm-cli, (but I think it forwards the arguments incorrectly; $args is a string, and that should be @Args).

$args is an array, why do you think it's a string? And yes there's also @Args, but that's not supported in all PS versions.

https://community.spiceworks.com/topic/686962-args-in-powershell#entry-4108731

This is pretty hard to follow. My gut is that proto shouldn't be using PowerShell and internal shims like this.

What do you suggest? It's either ps1 files or cmd/bat files, but ps1 is just far nicer to work with.

@rotu
Copy link
Contributor Author

rotu commented Oct 30, 2023

To start, the global shim triggers proto run ... so that the runtime version detection works.

The local shim does not use proto run and instead executes the .js file directly (using node as the parent binary) to avoid the overhead again. While this is true for npm (in this example), the node binary in the global shim may still trigger it's own version detection for Node.js.

There's still the overhead of starting a PowerShell. I'm not understanding why,

$args is an array, why do you think it's a string? And yes there's also @Args, but that's not supported in all PS versions.

https://community.spiceworks.com/topic/686962-args-in-powershell#entry-4108731

Oops. You're right - $args is an array. But I still think the shim is mangling the arguments with npm set 'editor=code -w', where -w is getting misinterpreted as its own argument to npm.

Running `target\debug\proto.exe run npm latest -- set "editor=code -w"`
[DEBUG 2023-10-29 23:53:12] proto  Running proto v0.21.0  args=["target\\debug\\proto.exe", "run", "npm", "latest", "--", "set", "editor=code -w"]
[DEBUG 23:53:27] proto::commands::run:run  Using local shim for tool  shim="/tmp/proto_test\\tools\\npm\\10.2.1\\shims\\npm.ps1"
[DEBUG 23:53:27] proto::commands::run:run  Running npm  bin="/tmp/proto_test\\tools\\npm\\10.2.1\\shims\\npm.ps1" args=["set", "editor=code -w"]
npm ERR! code ENOWORKSPACES
npm ERR! This command does not support workspaces.

What do you suggest? It's either ps1 files or cmd/bat files, but ps1 is just far nicer to work with.

I think I'm missing something pretty core here.

In run.rs, why not just call Command::new(node_exe) and pass the path to npm-cli.js there (or, more likely, do this in the node-plugin in some function like "invoke_tool")? Why do you think there needs to be a shell here at all?

@milesj
Copy link
Contributor

milesj commented Oct 30, 2023

In run.rs, why not just call Command::new(node_exe) and pass the path to npm-cli.js there (or, more likely, do this in the node-plugin in some function like "invoke_tool")? Why do you think there needs to be a shell here at all?

Mainly because it has to be generic enough to work for all tools that could possibly exist. All of this shim stuff has existed before plugins existed, so some of it is just legacy at this point.

But as for Windows specifically, if we end up having to execute .cmd or .ps1 files from the tools themselves (npm.cmd for example), the shell will still be required.

But yes, this is why proto is still a v0, as all of these features need to be ironed out.

@rotu
Copy link
Contributor Author

rotu commented Oct 30, 2023

Mainly because it has to be generic enough to work for all tools that could possibly exist. All of this shim stuff has existed before plugins existed, so some of it is just legacy at this point.

But as for Windows specifically, if we end up having to execute .cmd or .ps1 files from the tools themselves (npm.cmd for example), the shell will still be required.

Okay, I think we're on the same page now. The scripts npm (with a bash shebang), npm.cmd, npm.ps1 are all shims (albeit installed by npm themselves) largely oriented toward finding the version of node they were bundled with. There may be a case where we do have to run the .cmd or .ps1 scripts bundled with a tool, but I don't think npm is that case.

But yes, this is why proto is still a v0, as all of these features need to be ironed out.

Happy to put it through its paces! I appreciate so much the work you do! I want this tool to be awesome and to improve the state of software and I hope that comes across!

This GitHub issue in particular is broadly written and not very clear.


I think the main takeaways are (and I could split these out into separate issues):

  1. A tool may not have a single entry points / bin / shim. When that relationship is not one-to-one, that relationship should be clearer and more discoverable (sounds like this is planned for something like proto plugin info <tool>, though I might go with proto tool <tool id> in case the plugin/tool relationship is also not one-to-one; BTW, proto --help and proto plugins are the best commands for discoverability because they don't expect me to already know what a "tool" is!).
  2. In the case of npx, the shim should properly be part of npm, not node.
  3. proto bin and proto run are oriented around "tools", not bins. Maybe clarify in docs. Or maybe don't bother if proto bin is going away.
  4. There are two levels of "shim" - global and local; the global shims serve as an interface between the user's shell and proto. The local shims are an interface between proto and the tool itself. (maybe clarify language in docs / --help text)

@milesj
Copy link
Contributor

milesj commented Oct 30, 2023

I'm tinkering with some of this here: #268

@milesj
Copy link
Contributor

milesj commented Nov 8, 2023

This should be much more straight forward now in 0.22.

@rotu
Copy link
Contributor Author

rotu commented Nov 8, 2023

I can see that:

  • local shims are now no more (yay!)
  • The npx shim now looks like proto.exe run npm --alt "npx" -- %* so it's now part of the npm tool (yay!)
  • npm set 'editor=code -w' now works as intended on Windows! (yay!)

I can see there's a little messiness in proto run, namely that the --alt and --bin options are confusingly similar, but I'm guessing one of them is going away eventually.

We also might want some clarifying language somewhere about the difference between a plugin, a tool, and a bin. I'm inferring the difference is:

  1. A plugin is the adapter layer that provides added functionality to proto. This typically has one tool (e.g. deno), might have multiple (e.g. node-depman) and might have none (e.g. the schema plugin)
  2. A tool is a set of features that you might want to install and it has a version which is independent from the version of other tools. Typically a tool has a main bin, but it may have other helpers that are versioned in lockstep (e.g. npm and npx come from the same npm tool).
  3. A bin is something which the operating system can execute.

At any rate, I think this issue can be closed as much less confusing now!

@milesj
Copy link
Contributor

milesj commented Nov 17, 2023

I've updated the docs a bit to clarify the terminology.

@milesj milesj closed this as completed Nov 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

No branches or pull requests

2 participants