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

proto run npm/pnpm/yarn fails with program not found if bin not in path #311

Closed
rotu opened this issue Nov 30, 2023 · 16 comments
Closed

proto run npm/pnpm/yarn fails with program not found if bin not in path #311

rotu opened this issue Nov 30, 2023 · 16 comments
Assignees
Labels
bug Something isn't working

Comments

@rotu
Copy link
Contributor

rotu commented Nov 30, 2023

What version?

0.23.5

Which command?

No response

What happened?

> proto run pnpm
Error:   × program not found

My path includes ~/.proto/shims but NOT ~/.proto/bin per here.

node_depman_plugin version 0.5.3
node_plugin version 0.5.3

Trace logs?

$ proto run pnpm
[DEBUG 2023-11-30 11:23:44] proto  Running proto v0.23.5  args=["C:\\Users\\dan\\.proto\\shims\\proto.exe", "run", "pnpm", "--log", "trace"]
[TRACE 23:44.785] starbase::app  Running startup phase with 1 systems
[TRACE 23:44.785] starbase::app  Running analyze phase with 1 systems
[TRACE 23:44.786] starbase::app  Running execute phase with 2 systems
[DEBUG 23:44.790] proto_core::user_config:run  Loading config.toml  file="C:\\Users\\dan\\.proto\\config.toml"
[TRACE 23:44.791] starbase_utils::fs:run  Reading file  file="C:\\Users\\dan\\.proto\\config.toml"
[DEBUG 23:44.791] proto_core::tool_loader:run  Traversing upwards to find a configured plugin  tool="pnpm"
[DEBUG 23:44.792] proto_core::tools_config:run  Loading .prototools  file="C:\\Users\\dan\\.prototools"
[TRACE 23:44.792] starbase_utils::fs:run  Reading file  file="C:\\Users\\dan\\.prototools"
[TRACE 23:44.802] warpgate::loader:run  Creating plugin loader  cache_dir="C:\\Users\\dan\\.proto\\plugins"
[TRACE 23:44.802] warpgate::loader:run  Loading plugin pnpm  plugin="pnpm"
[TRACE 23:44.803] warpgate::loader:run  Plugin already downloaded and cached  plugin="pnpm" path="C:\\Users\\dan\\.proto\\plugins\\pnpm-50fe3b6c66bfa9dd0663411862e9dd8fb7e301f8387f8dcd03a890094c977902.wasm"
[DEBUG 23:44.803] proto_core::tool_loader:run  Loading WASM plugin  source="C:\\Users\\dan\\.proto\\plugins\\pnpm-50fe3b6c66bfa9dd0663411862e9dd8fb7e301f8387f8dcd03a890094c977902.wasm"
[TRACE 23:44.804] proto_core::tool_loader:run  Storing tool identifier  id="pnpm"
[TRACE 23:44.804] proto_core::tool_loader:run  Storing user configuration  config={"auto_clean":true,"auto_install":true,"node_intercept_globals":true}
[TRACE 23:44.805] proto_core::tool_loader:run  Storing proto environment  env={"arch":"x64","os":"windows","home_dir":{"path":"/userhome/","virtual_prefix":"/userhome","real_prefix":"C:\\Users\\dan"},"proto_dir":{"path":"/proto/","virtual_prefix":"/proto","real_prefix":"C:\\Users\\dan\\.proto"}}
[DEBUG 23:44.805] proto_core::tool:run  Creating tool pnpm and instantiating plugin
[DEBUG 23:44.806] proto_core::tool_manifest:run:load  Loading manifest.json  file="C:\\Users\\dan\\.proto\\tools\\pnpm\\manifest.json"
[TRACE 23:44.806] starbase_utils::fs:run:load  Opening file  file="C:\\Users\\dan\\.proto\\tools\\pnpm\\manifest.json"
[TRACE 23:44.807] starbase_utils::fs_lock:run:load  Locking file  file="C:\\Users\\dan\\.proto\\tools\\pnpm\\manifest.json"
[TRACE 23:44.808] starbase_utils::fs_lock:run:load  Unlocking file  file="C:\\Users\\dan\\.proto\\tools\\pnpm\\manifest.json"
[DEBUG 23:45.017] proto_core::tool:run  Created tool pnpm and its WASM runtime
[TRACE 23:45.018] warpgate::plugin:run  Calling plugin function register_tool  plugin="pnpm" input={"id":"pnpm"}
[TRACE 23:45.019] warpgate::plugin:run  Called plugin function register_tool  plugin="pnpm" output={"inventory":{},"name":"pnpm","plugin_version":"0.5.3","type":"DependencyManager"}
[TRACE 23:45.019] proto_core::version_detector:run  Attempting to find local version from config files  tool="pnpm"
[DEBUG 23:45.020] proto_core::user_config:run  Loading config.toml  file="C:\\Users\\dan\\.proto\\config.toml"
[TRACE 23:45.020] starbase_utils::fs:run  Reading file  file="C:\\Users\\dan\\.proto\\config.toml"
[TRACE 23:45.021] proto_core::version_detector:run  Checking directory  tool="pnpm" dir="C:\\Users\\dan"
[DEBUG 23:45.021] proto_core::tools_config:run  Loading .prototools  file="C:\\Users\\dan\\.prototools"
[TRACE 23:45.022] starbase_utils::fs:run  Reading file  file="C:\\Users\\dan\\.prototools"
[DEBUG 23:45.023] proto_core::version_detector:run  Detected version from .prototools file  tool="pnpm" version="latest"
 file="C:\\Users\\dan\\.prototools"
[DEBUG 23:45.024] proto_core::user_config:run  Loading config.toml  file="C:\\Users\\dan\\.proto\\config.toml"
[TRACE 23:45.025] starbase_utils::fs:run  Reading file  file="C:\\Users\\dan\\.proto\\config.toml"
[DEBUG 23:45.025] proto_core::tool:run  Resolving a semantic version or alias  tool="pnpm" initial_version="latest"
[DEBUG 23:45.026] proto_core::tool:run  Loading available versions  tool="pnpm"
[TRACE 23:45.026] starbase_utils::fs:run  Reading file metadata  file="C:\\Users\\dan\\.proto\\tools\\pnpm\\remote-versions.json"
[DEBUG 23:45.027] proto_core::tool:run  Loading from local cache  tool="pnpm" cache="C:\\Users\\dan\\.proto\\tools\\pnpm\\remote-versions.json"
[TRACE 23:45.027] starbase_utils::fs:run  Reading file  file="C:\\Users\\dan\\.proto\\tools\\pnpm\\remote-versions.json"
[TRACE 23:45.028] starbase_utils::json:run  Parsing JSON  file="C:\\Users\\dan\\.proto\\tools\\pnpm\\remote-versions.json"
[TRACE 23:45.029] warpgate::plugin:run  Calling plugin function resolve_version  plugin="pnpm" input={"initial":"latest"}
[TRACE 23:45.030] warpgate::plugin:run  Called plugin function resolve_version  plugin="pnpm" output={}
[DEBUG 23:45.030] proto_core::tool:run  Resolved to 8.11.0  tool="pnpm" version="8.11.0"
[DEBUG 23:45.031] proto_core::tool:run  Checking if tool is installed  tool="pnpm" install_dir="C:\\Users\\dan\\.proto\\tools\\pnpm\\8.11.0"
[DEBUG 23:45.032] proto_core::tool:run  Tool has already been installed, locating binaries and shims  tool="pnpm" install_dir="C:\\Users\\dan\\.proto\\tools\\pnpm\\8.11.0"
[DEBUG 23:45.032] proto_core::tool:run  Locating executable for tool  tool="pnpm"
[TRACE 23:45.033] warpgate::plugin:run  Calling plugin function locate_executables  plugin="pnpm" input={"context":{"tool_dir":{"path":"/proto/tools/pnpm/8.11.0","virtual_prefix":"/proto","real_prefix":"C:\\Users\\dan\\.proto"},"version":"8.11.0"}}
[TRACE 23:45.034] warpgate::plugin:run  Called plugin function locate_executables  plugin="pnpm" output={"globals_lookup_dirs":["$PROTO_HOME/tools/node/globals/bin"],"primary":{"exe_path":"bin/pnpm.cjs","no_bin":true,"parent_exe_name":"node"},"secondary":{"pnpx":{"no_bin":true,"shim_before_args":"dlx"}}}
[DEBUG 23:45.034] proto_core::tool:run  Found an executable  tool="pnpm" exe_path="C:\\Users\\dan\\.proto\\tools\\pnpm\\8.11.0\\bin/pnpm.cjs"
[TRACE 23:45.035] warpgate::plugin:run  Calling plugin function pre_run  plugin="pnpm" input={"context":{"tool_dir":{"path":"/proto/tools/pnpm/8.11.0","virtual_prefix":"/proto","real_prefix":"C:\\Users\\dan\\.proto"},"version":"8.11.0"},"passthrough_args":[]}
[TRACE 23:45.036] warpgate::plugin:run  Called plugin function pre_run  plugin="pnpm" output=
[DEBUG 23:45.036] proto::commands::run:run  Running pnpm  bin="node.exe" args=["C:\\Users\\dan\\.proto\\tools\\pnpm\\8.11.0\\bin/pnpm.cjs"]
[TRACE 23:45.042] starbase::app  Running shutdown phase with 1 systems
Error:   × program not found

Operating system?

Windows

Architecture?

x64

@rotu rotu added the bug Something isn't working label Nov 30, 2023
@rotu
Copy link
Contributor Author

rotu commented Nov 30, 2023

Current workaround in my PowerShell profile:

function pnpm { proto run node -- $(proto bin pnpm) @args }
function npm { proto run node -- $(proto bin npm) @args }
function yarn { proto run node -- $(proto bin yarn) @args }

Also needed this for powershell-based app kickers installed by npm at node_modules/.bin/*.ps1, since on Windows, these invoke node with an explicit .exe suffix:

function node.exe { proto run node -- @args }

@milesj
Copy link
Contributor

milesj commented Nov 30, 2023

@rotu Ah damn. This is a troublesome one.

In the last PR (#308), I made the change that tools that require a parent (mainly npm/pnpm/yarn) will be executed with the parent .exe. But since you removed the ~/.proto/bin folder, that .exe is no longer available.

This change was made because otherwise, we have nested shim executions, which cause problems with CTRL+C and signals on Windows. For example, this is a standard flow.

  1. shim: npm install -> Runs ~/.proto/shims/npm.cmd, which in turn runs proto run npm -- install.
  2. not shim: proto run npm -- install -> Requires node, so runs node ~/.proto/tools/npm/.../cli.js install under the hood.
  3. shim: node ~/.proto/tools/npm/.../cli.js install -> Runs ~/.proto/shims/node.cmd ..., which causes a bunch of problems.
  4. not shim: proto run node -- ~/.proto/tools/npm/.../cli.js install.

As you can see, it kind of just keeps going all the way down. The intermediate shims are problematic, so I updated it to skip the shims.

I'm open to feedback on this. Windows shims seem to be causing a bunch of problems and I'm not 100% sure what the ideal path forward is now. Some ideas:

  • Add settings for controlling which shims are created: .cmd, .ps1, etc.
  • Add settings for toggling shims/bin creation of or off.
  • Investigate a new shim pattern for Windows.

@rotu
Copy link
Contributor Author

rotu commented Nov 30, 2023

@rotu Ah damn. This is a troublesome one.

In the last PR (#308), I made the change that tools that require a parent (mainly npm/pnpm/yarn) will be executed with the parent .exe. But since you removed the ~/.proto/bin folder, that .exe is no longer available.

Is nested shim execution still a problem now that "Terminate batch job?" doesn't show up? What's wrong with calling shims/node.cmd in step 3?

I thought the "terminate batch job?" was the main shim issue. Admittedly I don't understand the cmd shim hack at all right now:

endlocal & goto #_undefined_# 2>NUL || title %COMSPEC% & proto.exe run node --  %* 

I think adding .exe is a bad idea. I'm thinking the right thing to do is either leave it unqualified (if nested shim execution actually does work) or to fully resolve node to the real executable in tools/node/...

I'm open to feedback on this. Windows shims seem to be causing a bunch of problems and I'm not 100% sure what the ideal path forward is now. Some ideas:

* Add settings for controlling which shims are created: .cmd, .ps1, etc.

* Add settings for toggling shims/bin creation of or off.

* Investigate a new shim pattern for Windows.

Here are my thoughts:

  1. We need an .exe shim in the path on Windows.
    • I currently hack this by having a powershell function called node.exe but even having a copy of the batch file called node.exe.cmd will work in a pinch!
    • An .exe shim doesn't necessarily need to be a new executable for each file. It could just be a symlink to proto.exe which acts based on argv[0] to delegate to the underlying tool binary.
  2. A good .exe shim may render the .cmd shim unnecessary.
  3. To that end, proto needs a way to map the name of a bin to the plugin/tool which provides it. I realize this is ambiguous (e.g. two plugins could declare a program with the same name) and proto needs to handle this either implicitly or by saving desired shims in the .prototools config.
  4. The PowerShell .ps1 shims seem currently pointless and I think should be removed. The only reason I might have preferred it is because of the ctrl-c issue.

@milesj
Copy link
Contributor

milesj commented Nov 30, 2023

Is nested shim execution still a problem now that "Terminate batch job?" doesn't show up? What's wrong with calling shims/node.cmd in step 3?

Yeah it was still a problem in my testing.

I thought the "terminate batch job?" was the main shim issue. Admittedly I don't understand the cmd shim hack at all right now:

The problem was 2 things: 1) hitting ctrl+c either wouldn't work, or you had to spam it constantly for the process to exit. for the most part, you had to open a new terminal window. 2) when it did exit with ctrl+c, the terminal history is lost for some reason. Also required a new terminal window.

I think adding .exe is a bad idea. I'm thinking the right thing to do is either leave it unqualified (if nested shim execution actually does work) or to fully resolve node to the real executable in tools/node/...

Had another idea. Maybe we resolve it to proto run node instead of node.exe. This assumes that the tool IDs match up though, which is probably ok for the node case.

A good .exe shim may render the .cmd shim unnecessary.

Agreed. I've been looking into creating the .exe at runtime, but it seems kind of... impossible. But I know its not. Maybe need another kind of portal executable.

The PowerShell .ps1 shims seem currently pointless and I think should be removed. The only reason I might have preferred it is because of the ctrl-c issue.

The .ps1 files actually work great. The problem is the .cmd files. And the other problem is that .ps1 is not in PATHEXT, so node wouldn't run node.ps1, only node.cmd.

We could modify PATHEXT on install, but that's a can of worms.

@milesj
Copy link
Contributor

milesj commented Nov 30, 2023

Testing here, this may work: #313

@rotu
Copy link
Contributor Author

rotu commented Nov 30, 2023

I thought the "terminate batch job?" was the main shim issue. Admittedly I don't understand the cmd shim hack at all right now:

The problem was 2 things: 1) hitting ctrl+c either wouldn't work, or you had to spam it constantly for the process to exit. for the most part, you had to open a new terminal window. 2) when it did exit with ctrl+c, the terminal history is lost for some reason. Also required a new terminal window.

That does sound crummy.

I found an explanation of the hack: microsoft/terminal#217

It creates a fatal error (goto an undefined label) and runs the command in the shell process right before it burns down! I'm not at all surprised it splatters the history!

I recommend taking another look at this alternate solution. It does seem to play nice with npx (I think it was an argument handling thing) and I think it will nest better. Instead of running the executable in the process as it's going down, you're directly intercepting the error code before it gets back to the shell and then exiting gracefully:

setlocal enabledelayedexpansion 
proto.exe run node -- %* & set RC=!errorlevel! & call;
EXIT /b %RC%

Had another idea. Maybe we resolve it to proto run node instead of node.exe. This assumes that the tool IDs match up though, which is probably ok for the node case.

Yeah! I think that's a very reasonable solution.

The .ps1 files actually work great. The problem is the .cmd files.

But the .cmd files are necessary since processes look for node which resolves to node.cmd. The only thing that uses the .ps1 files is a PowerShell session, where a function or Cmdlet provides a better user experience anyway.

And the other problem is that .ps1 is not in PATHEXT, so node wouldn't run node.ps1, only node.cmd.

We could modify PATHEXT on install, but that's a can of worms.

You need to ensure .ps1 is in PATHEXT (so the user can omit the extension), ensure that the default shell handler is to execute the script file with PowerShell and not edit it with a text editor (maybe not a great idea?), and make sure the script gets executed in the current PowerShell process or a subshell, not a new window.

@milesj
Copy link
Contributor

milesj commented Dec 1, 2023

Just had a thought, thanks to this comment:

An .exe shim doesn't necessarily need to be a new executable for each file. It could just be a symlink to proto.exe which acts based on argv[0] to delegate to the underlying tool binary.

Maybe, just maybe, we can provide 2 executables: proto(.exe) and proto-shim(.exe). This could use the argv[0] hack to "determine the correct tool" to run, and simply call proto under the hood. We could probably then eliminate bash/ps1/cmd shims entirely for both OS's.

@rotu
Copy link
Contributor Author

rotu commented Dec 1, 2023

Just had a thought, thanks to this comment:

An .exe shim doesn't necessarily need to be a new executable for each file. It could just be a symlink to proto.exe which acts based on argv[0] to delegate to the underlying tool binary.

Maybe, just maybe, we can provide 2 executables: proto(.exe) and proto-shim(.exe). This could use the argv[0] hack to "determine the correct tool" to run, and simply call proto under the hood. We could probably then eliminate bash/ps1/cmd shims entirely for both OS's.

Now we're on the same page! I said one executable, but I don't have a strong feeling about one or two. This might be able to fully replace the current shims, but there's some references to platform-specific behavior, so I'm not sure where it wouldn't work (or if we even care about those platforms):

If the executable was invoked through a symbolic link, some platforms will return the path of the symbolic link and other platforms will return the path of the symbolic link’s target.

and here

The first element is traditionally the path of the executable, but it can be set to arbitrary text, and might not even exist. This means this property should not be relied upon for security purposes.

@milesj
Copy link
Contributor

milesj commented Dec 1, 2023

Yeah I'll do some testing and see what's feasible. I was thinking a secondary shim just so it's super lightweight.

As for this issue, can you try 0.23.6 and see if that works?

@milesj
Copy link
Contributor

milesj commented Dec 1, 2023

Testing some stuff here. #315

If there's certain flows you think I should test (besides signals), let me know.

@rotu
Copy link
Contributor Author

rotu commented Dec 1, 2023

As for this issue, can you try 0.23.6 and see if that works?

I'll give it a shot when I'm back on my Windows compy, probably tomorrow.

If there's certain flows you think I should test (besides signals), let me know.

A couple things:

  1. piping to the shim
  2. killing the shim
  3. killing a subprocess of the shim
  4. running the shim via an alias
  5. running a python venv

@milesj
Copy link
Contributor

milesj commented Dec 1, 2023

Good news, both unix/windows resolves the arg name correctly. Also got ctrl+c working super easily. https://github.com/moonrepo/proto/blob/0.24-shim/crates/cli-shim/README.md

I'll test piping/redirects etc too, but this seems like a good win. The file size is also 400kb.

@rotu
Copy link
Contributor Author

rotu commented Dec 1, 2023

Yeah I'll do some testing and see what's feasible. I was thinking a secondary shim just so it's super lightweight.

As for this issue, can you try 0.23.6 and see if that works?

It works! Only two minor issues:

  1. Since I moved proto.exe to shims, proto upgrade merely installs the new proto instance to bin which is not in my path. Recommend instead symlinking proto.exe into my path:
New-Item -Path $env:PROTO_HOME\shims\proto.exe -ItemType SymbolicLink -Value $env:PROTO_HOME\bin\proto.exe
  1. After uninstalling node and npm, proto run npm did not reinstall node. This is probably okay.

@milesj
Copy link
Contributor

milesj commented Dec 1, 2023

Was going to land this change in the next moon version, but I can probably land it sooner: https://github.com/moonrepo/moon/pull/1208/files#diff-8804ae5ea9955fa400fa6ff23b4e911bb4fc5451e09244b0beec7aac21286222R25

Can update upgrade to use that as well.

@rotu
Copy link
Contributor Author

rotu commented Dec 1, 2023

Was going to land this change in the next moon version

I don't have a strong feeling about this but I'm excited that moon is making progress!

Admittedly, I still don't understand moon and I don't use it in any meaningful way! I work on a npm-based monorepo project under Mercurial source control. I can put proto through its paces, but not (yet) moon.

@milesj
Copy link
Contributor

milesj commented Dec 1, 2023

It was mainly about adding the PROTO_INSTALL_DIR env var. This way, you can install it to a custom location.

I'm going to close this. Let's keep the shim convo in that new PR.

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