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

Add shims for Volta-like binaries #609

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from
Draft

Add shims for Volta-like binaries #609

wants to merge 10 commits into from

Conversation

Schniz
Copy link
Owner

@Schniz Schniz commented Dec 21, 2021

This PR introduces --with-shims to the fnm env command.
These shims (currently only node, but maybe npm will need it too) will be used instead of the $FNM_MULTISHELL_PATH, and although they will introduce some runtime penalty (~8-16ms on my machine), it will be much easier to integrate with other tools and make people understand how fnm works.
This should also fix some rehashing issues some users have on newer Zsh versions.

I am still not sure this will fix the graphic IDE issue,
but it will most certainly fix the rehashing issues.

Still need to consider how to tackle the IDE issue.
Hmm.
Maybe I have the strangest idea.
I will try.
…ew PID so we can't ensure a consistent symlink path"

This reverts commit d1f7b9d.
@Schniz Schniz self-assigned this Dec 21, 2021
@vercel
Copy link

vercel bot commented Dec 21, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/schniz/fnm/CkggR7pGuDDtUcAG2qtKPoM8JYHL
✅ Preview: https://fnm-git-add-with-shims-schniz.vercel.app

@Schniz
Copy link
Owner Author

Schniz commented Dec 21, 2021

d1f7b9d is because I thought I would be able to change the symlink generation process from random to using the parent process' pid, and therefore making it reproducible, and making fnm env not mandatory at all (unless one wants fancy features).

Unfortunately, every shell script has its own pid so we need to find a common denominator somehow. Shells are obviously not the common denominator (the shims are a shell script), so we need to know whether it's an interactive shell--or a graphical IDE (like #475 and other issues).

If someone has an idea how to pull this off, let me know 😃

@Schniz
Copy link
Owner Author

Schniz commented Dec 21, 2021

Shells are obviously not the common denominator (the shims are a shell script)

CLI (pid1) -> node shim (pid2) -> fnm (pid3)

fnm tries to take pid2, instead of pid1. The same can be applied in VSCode or XCode or whatever:

VSCode (pid1) -> some 3rd party script (pid2) -> another 3rd party (pid3) -> fnm (pid4)

and now we're trying to use pid3, which can be a one-off thing. So pids are probably not what we want. I also thought about implementing a table of pids to versions but this is just insanity and composes very very badly (how up the tree we need to update version when we switch?) 😁

@Schniz
Copy link
Owner Author

Schniz commented Dec 21, 2021

The premise of reproducible "multishell" paths is good, because it simplifies the installation of fnm, which can be awesome.

@Schniz Schniz marked this pull request as ready for review December 21, 2021 17:26
@@ -0,0 +1,3 @@
#!/usr/bin/env sh

fnm exec --using-current -- node "$@"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be you should use exec fnm exec ... so fnm's process replaces shell and process tree would be like this:

CLI (pid1) -> node shim (pid2) -> fnm (pid2)

Not sure how to do this in windows.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh! TIL!

this is still the problem because we have unknown parents that can bridge it. :(

shell -> scripts/run-tests.sh -> scripts/run-unit-tests.sh -> fnm exec ...

pids don't solve it 😭

@ljharb
Copy link

ljharb commented Dec 23, 2021

Shims are a fundamentally different model than PATH hijacking, and they come with an entirely distinct set of problems; I’ll be very curious to see how fnm deals with both at once.

@alexeyten
Copy link
Contributor

I've read some linked issues and it seems that people want fnm to install node globally (or behave like it's installed globally). Am I correct?

@alexeyten
Copy link
Contributor

What about this shim?

#!/bin/sh

cwd=$PWD

while [ -n "$cwd" ]; do
    if [ -f "$cwd/.node-version" ]; then
        # echo "Found $cwd/.node-version"
        exec fnm exec --using "$cwd/.node-version" -- node "$@"
    fi
    if [ -f "$cwd/.nvmrc" ]; then
        # echo "Found $cwd/.nvmrc"
        exec fnm exec --using "$cwd/.nvmrc" -- node "$@"
    fi
    cwd=${cwd%/*}
done

exec fnm exec --using default -- node "$@"

@Schniz
Copy link
Owner Author

Schniz commented Dec 23, 2021

We can have fnm exec --fallback-to-default but I am not sure this is enough.. what if someone wants to change a node version?

I just wonder if there's a better option we haven't though of, that will make fnm env optional

@alexeyten
Copy link
Contributor

what if someone wants to change a node version?

You meant beside changing default version?

docs/commands.md Outdated
@@ -194,6 +194,9 @@ FLAGS:
-V, --version
Prints version information

--with-shims
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems that this args is removed at the latest version?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it was never added, I'm gonna add this with a proper warning, I will test it on my machine for now :relieved-telegram:

@changeset-bot
Copy link

changeset-bot bot commented Nov 17, 2022

⚠️ No Changeset found

Latest commit: 2299a02

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@Schniz
Copy link
Owner Author

Schniz commented Nov 18, 2022

main problem I see with it right now, that it will make all global node module installations fail, as they install binaries in $NODE_INSTALL_DIR/bin, and we're not linking it to $PATH :sadblob:

gotta dig deeper :dig-deep:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: New Feature A new feature was added
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants