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

fix: resolve symbolic links completely when hunting for subcommands #935

Merged
merged 1 commit into from
Apr 1, 2019

Conversation

MarshallOfSound
Copy link
Contributor

Depending on your npm / yarn set up a global install may have to resolve through several symlinks before it correctly arrives the the baseDir that actually contains the other subcommands. The links go between directories so if we stop after the first link baseDir is looking in the wrong place

The changes the resolution logic to resolve symlinks until their are no more to resolve by using realpath which will resolve links all the way to the final destination.

index.js Outdated Show resolved Hide resolved
Depending on your npm / yarn set up a global install may have to resolve
through several symlinks before it correctly arrives the the baseDir
that actually contains the other subcommands.

The changes the resolution logic to resolve symlinks until their are no
more to resolve by using realpath which will resolve links all the way
to the final destination.
@shadowspawn
Copy link
Collaborator

Might take me a few days, but I am looking forward to reviewing this. Hopefully fixes #866 (and without adding a dependency as in #869).

@MarshallOfSound could you please suggest some reproduction steps that match how a user might end up with the broken symlink? It sounded like you had something in mind:

Depending on your npm / yarn set up a global install may have to resolve through several symlinks before it correctly arrives the the baseDir that actually contains the other subcommands.

I did find mention of yarn install in #866 (comment) and will be trying that too.

@MattCheely
Copy link

@MarshallOfSound could you please suggest some reproduction steps that match how a user might end up with the broken symlink? It sounded like you had something in mind:

I'm not the OP, but I have a package that you can replicate the issue with via:

yarn global add elm-app-gen

and then run:

elm-app-gen quickstart test-app

in some temporary directory.

@MarshallOfSound
Copy link
Contributor Author

@shadowspawn Anyone using subcommands and the latest version of yarn will have this issue afaik.

Yarn installs the binary to /usr/local/bin/my-cmd which symlinks to $YARN_DIR/.bin/my-cmd which then symlinks to $YARN_DIR/node_modules/my-package/bin/my-cmd.

You can repro this quite easily with yarn global add @electron-forge/cli && electron-forge init t in some temporary directory

@KillWolfVlad
Copy link
Contributor

@MarshallOfSound I did not know about fs.realpath. Your solution looks better and more native.

@MarshallOfSound MarshallOfSound changed the title fix: resolve symbol links completely when hunting for subcommands fix: resolve symbolic links completely when hunting for subcommands Mar 25, 2019
@shadowspawn
Copy link
Collaborator

(My review progress: examined code and related issues and tested on Mac, looking good, Windows still to go.)

Copy link
Collaborator

@shadowspawn shadowspawn left a comment

Choose a reason for hiding this comment

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

Thanks, tidy.

@MarshallOfSound
Copy link
Contributor Author

@shadowspawn What needs to happen to get this merged and released?

@shadowspawn
Copy link
Collaborator

@MarshallOfSound
I joined as collaborator recently and not involved in the release process itself yet. Nothing more needed from you at the moment though.

@abetomo
Copy link
Collaborator

abetomo commented Apr 1, 2019

Thank you!
We will release it soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants