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 handling of cli arg that is symlink #659

Merged
merged 4 commits into from
Jun 14, 2021

Conversation

jeremywiebe
Copy link
Collaborator

Summary
I've had trouble running checksync for quite a while. I had to resort to running it by passing the entrypoint file to node manually node path/to/checksync.js.

The error I'd see is this:

(node:45761) UnhandledPromiseRejectionWarning: Error: ENOTDIR: not a directory, scandir '/Users/jeremy/.volta/tools/image/packages/checksync/bin/checksync'
(node:45761) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 1)
(node:45761) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

Issues
I haven't raised an issue for this. I can, if you'd like. :)

Details

I finally traced the error to how symlinks are resolved for unrecognized args.

The process sees a __filename of /Users/jeremy/.volta/tools/image/packages/checksync/bin/checksync. On my system, fs.readlinkSync(__filename) returns ../lib/node_modules/.bin/checksync. When path.resolve() resolves that path, it ends up at a path that doesn't exist on my system because it joins process.cwd() with the relative path passed to path.resolve().

I think the fix is just to use fs.realpathSync() which appears to resolve to the right path on my system.

Screenshots
If applicable, add screenshots to help illustrate your changes

Review notes
Add additional context for reviewers here. Consider what, if anything, needs particular attention paid to it.
If this is a draft, perhaps clarify what work remains and what, if anything, is ready for review.

@codecov
Copy link

codecov bot commented Jun 12, 2021

Codecov Report

Merging #659 (380fc1b) into main (29903e5) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #659   +/-   ##
=======================================
  Coverage   99.81%   99.81%           
=======================================
  Files          30       30           
  Lines         549      549           
  Branches      108      108           
=======================================
  Hits          548      548           
  Misses          1        1           
Impacted Files Coverage Δ
src/cli.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 29903e5...380fc1b. Read the comment docs.

Copy link
Owner

@somewhatabstract somewhatabstract left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -75,7 +74,7 @@ export const run = (launchFilePath: string): void => {
if (arg.endsWith(".bin/checksync")) return false;
// Handle the entry point being a symlink
try {
const realpath = path.resolve(fs.readlinkSync(arg));
const realpath = fs.realpathSync(arg);
Copy link
Owner

Choose a reason for hiding this comment

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

I cannot remember why I did not use this originally - I do remember there was a reason. lol

Helpful, huh?

@somewhatabstract somewhatabstract merged commit e8b3ee2 into somewhatabstract:main Jun 14, 2021
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.

2 participants