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

new: Create a new shim executable. #315

Merged
merged 10 commits into from
Dec 13, 2023
Merged

new: Create a new shim executable. #315

merged 10 commits into from
Dec 13, 2023

Conversation

milesj
Copy link
Contributor

@milesj milesj commented Dec 1, 2023

No description provided.

@rotu
Copy link
Contributor

rotu commented Dec 1, 2023

Go Miles! I'm super excited to have a shim solution that's not specific to the shell!

@milesj
Copy link
Contributor Author

milesj commented Dec 1, 2023

@rotu So far this works perfectly for primary shims (npm, node, etc), but unfortunately doesn't work for secondary shims (npx, pip, etc).

The problem is that secondary shims add --alt bin flags and before/after args into the shell file, which are then passed to proto. When relying on argv[0], we have lost this information.

The only solution I can think of is having some kind of manifest like ~/.proto/shims/manifest.json that keeps a mapping of how everything should be executed.

@rotu
Copy link
Contributor

rotu commented Dec 1, 2023

@rotu So far this works perfectly for primary shims (npm, node, etc), but unfortunately doesn't work for secondary shims (npx, pip, etc).

The problem is that secondary shims add --alt bin flags and before/after args into the shell file, which are then passed to proto. When relying on argv[0], we have lost this information.

The only solution I can think of is having some kind of manifest like ~/.proto/shims/manifest.json that keeps a mapping of how everything should be executed.

Agreed, but I think secondary shims are a problem from the beginning. That's what I meant in my comment:

  1. 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.

I think the easiest thing is to add a section to the yaml, which maps the name to a $PROTO_HOME-relative path:

[shims]
npm = "tools/npm/10.2.4/bin/npm"

This even provides the ability to shim to the real bundled npm instead of one provided by node_depman_plugin:

[shims]
npm = "tools/node/20.10.0/bin/npm"

If these need to float with the version, the path could instead be, e.g. tools/npm/${PROTO_NPM_VERSION}/bin/npm or use some other placeholder like proto:npm/bin/npm.

@milesj
Copy link
Contributor Author

milesj commented Dec 1, 2023

Yup exactly. Will look into ways to easily solve this. This may land in 0.25 instead of 0.24. Want to ensure it all works correctly.

@rotu
Copy link
Contributor

rotu commented Dec 1, 2023

This also allows you to e.g. remap node like bunx --bun does:

[shims]
node = path/to/bun

or even possibly configure arguments on a per-project basis:

[shims]
node = ["path/to/node", "--loader", "ts-node/esm"]

Base automatically changed from develop-0.24 to master December 6, 2023 19:15
@milesj milesj changed the base branch from master to develop-0.26 December 13, 2023 02:56
@milesj milesj marked this pull request as ready for review December 13, 2023 04:50
@milesj milesj merged commit 4b382d7 into develop-0.26 Dec 13, 2023
@milesj milesj deleted the 0.24-shim branch December 13, 2023 04:55
milesj added a commit that referenced this pull request Dec 14, 2023
milesj added a commit that referenced this pull request Dec 14, 2023
milesj added a commit that referenced this pull request Dec 14, 2023
milesj added a commit that referenced this pull request Dec 15, 2023
milesj added a commit that referenced this pull request Dec 21, 2023
milesj added a commit that referenced this pull request Dec 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants