FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

node: warn if cwd is inaccessible during bootstrap by Fishrock123 · Pull Request #12022 · nodejs/node · GitHub

/ node Public

node: warn if cwd is inaccessible during bootstrap - #12022

Closed
Fishrock123 wants to merge 1 commit into
nodejs:masterfrom
Fishrock123:warn-on-inaccesible-cwd
ConversationCommits (1)ChecksFiles changed
Closed

node: warn if cwd is inaccessible during bootstrap#12022
Fishrock123 wants to merge 1 commit into
nodejs:masterfrom
Fishrock123:warn-on-inaccesible-cwd

Conversation

Fishrock123 commented Mar 24, 2017

Copy link
Copy Markdown
Contributor
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

bootstrap

This was added/fixed in 2c6f79c and seems like it could be quite confusing, so... maybe we should warn? ¯\_(ツ)_/¯

I don't really know how to trigger this though, so no test... for now?

cc @bnoordhuis, maybe this isn't a good idea, idk.

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Mar 24, 2017
Fishrock123 force-pushed the warn-on-inaccesible-cwd branch from cee2c9b to f381a70 Compare March 24, 2017 15:54

addaleax commented Mar 24, 2017

Copy link
Copy Markdown
Member

I don't really know how to trigger this though, so no test... for now?

mkdir /tmp/a; cd /tmp/a; rmdir /tmp/a; node -p 'module.paths'

Fishrock123 commented Mar 24, 2017

Copy link
Copy Markdown
Contributor Author

Ah ok, will write a test in a bit.

cjihrig requested changes Mar 24, 2017
View reviewed changes
Comment thread lib/internal/bootstrap_node.js Outdated

cjihrig Mar 24, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can you lowercase Current Working Directory.

Comment thread lib/internal/bootstrap_node.js Outdated

cjihrig Mar 24, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can you drop the last two dots.

jasnell requested changes Mar 24, 2017
View reviewed changes
Comment thread lib/internal/bootstrap_node.js Outdated

jasnell Mar 24, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

comma at the end of this line... did you mean to put a + here instead?

Fishrock123 commented Mar 27, 2017

Copy link
Copy Markdown
Contributor Author

ok, looks like there's more to it than this. Something loads path during startup which explodes regardless:

path.js:1163
          cwd = process.cwd();
                        ^

Error: ENOENT: no such file or directory, uv_cwd
    at Object.resolve (path.js:1163:25)
    at Function.Module._initPaths (module.js:642:22)
    at module.js:683:8
    at NativeModule.compile (bootstrap_node.js:548:7)
    at Function.NativeModule.require (bootstrap_node.js:489:18)
    at evalScript (bootstrap_node.js:390:33)
    at run (bootstrap_node.js:121:11)
    at run (bootstrap_node.js:445:7)
    at startup (bootstrap_node.js:120:9)
    at bootstrap_node.js:560:3

Fishrock123 commented Mar 27, 2017

Copy link
Copy Markdown
Contributor Author

I think the solution there would probably be to overload process.cwd() entirely on load?

Fishrock123 force-pushed the warn-on-inaccesible-cwd branch from f381a70 to f14c96f Compare March 27, 2017 16:43

cjihrig commented Apr 11, 2017

Copy link
Copy Markdown
Contributor

The cwd being removed is only supported for evaluating code. Not running "normally." I added a known issue test in #12343. We could add a better message for other scenarios, or set the cwd like we do for eval script.

cjihrig added a commit to cjihrig/node that referenced this pull request Apr 18, 2017
test: add cwd ENOENT known issue test
4fc1199 
If the current working directory is removed, Node cannot
start normally because the module system calls uv_cwd().

Refs: nodejs#12022
PR-URL: nodejs#12343
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR added the stalled Issues and PRs that are stalled. label Aug 26, 2017

BridgeAR commented Aug 26, 2017

Copy link
Copy Markdown
Member

Ping @Fishrock123

BridgeAR commented Sep 12, 2017

Copy link
Copy Markdown
Member

Closing this due to a long inactivity. Please feel free to reopen if you want to follow up on this @Fishrock123

BridgeAR closed this Sep 12, 2017
Trott pushed a commit to Trott/io.js that referenced this pull request Sep 29, 2017
test: add cwd ENOENT known issue test
1f4b70b 
If the current working directory is removed, Node cannot
start normally because the module system calls uv_cwd().

Refs: nodejs#12022
PR-URL: nodejs#12343
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 10, 2017
test: add cwd ENOENT known issue test
0597d3f 
If the current working directory is removed, Node cannot
start normally because the module system calls uv_cwd().

Backport-PR-URL: #15479
Refs: #12022
PR-URL: #12343
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 25, 2017
test: add cwd ENOENT known issue test
d8f5637 
If the current working directory is removed, Node cannot
start normally because the module system calls uv_cwd().

Backport-PR-URL: #15479
Refs: #12022
PR-URL: #12343
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell requested changes
cjihrig cjihrig requested changes

Assignees

No one assigned

Labels

lib / src Issues and PRs related to general changes in the lib or src directory. stalled Issues and PRs that are stalled.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL