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

fs: export realpathCacheKey from internal/fs #8862

Closed

Conversation

addaleax
Copy link
Member

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

fs

Description of change

Move the internally defined symbol fs.realpathCacheKey to the internal fs module, where it’s more appropriate.

The symbol was recently added in c084287, but since internal/fs is only available in the v7.x branch, this needs to be a separate follow-up change.

@addaleax addaleax added fs Issues and PRs related to the fs subsystem / file system. module Issues and PRs related to the module subsystem. dont-land-on-v4.x labels Sep 30, 2016
@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. module Issues and PRs related to the module subsystem. labels Sep 30, 2016
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM

@addaleax addaleax self-assigned this Oct 6, 2016
@addaleax
Copy link
Member Author

addaleax commented Oct 6, 2016

@addaleax addaleax removed their assignment Oct 6, 2016
@addaleax
Copy link
Member Author

addaleax commented Oct 6, 2016

@jasnell
Copy link
Member

jasnell commented Oct 7, 2016

@addaleax .. yeah, I've been spotting that one also. appears unrelated to this change.

@addaleax
Copy link
Member Author

addaleax commented Oct 7, 2016

So… trying again:

CI: https://ci.nodejs.org/job/node-test-commit/5502/

@addaleax
Copy link
Member Author

addaleax commented Oct 8, 2016

The FreeBSD failures are a bit worrying (although they can’t really be the effect of this change?)…

I think @jbergstroem reset one of the machines there, so maybe that’s it…

FreeBSD CI, master: https://ci.nodejs.org/job/node-test-commit-freebsd/4695/nodes=freebsd10-64/
FreeBSD CI, this PR: https://ci.nodejs.org/job/node-test-commit-freebsd/4696/nodes=freebsd10-64/

@jasnell
Copy link
Member

jasnell commented Oct 8, 2016

The freebsd issues have been persistent over the past week or so. I'd say that those are unrelated and that it should be safe to land this.

@addaleax
Copy link
Member Author

addaleax commented Oct 8, 2016

The freebsd issues have been persistent over the past week or so.

The issues in #8949 are different, unfortunately.

I'd say that those are unrelated and that it should be safe to land this.

I’d agree, this patch can’t really cause them. But since it looks like the failures are realpath-related, I’d really like to be careful…

They might also hint at a problem with the setup of one of the FreeBSD machines (or point to an actual, already-existing bug in Node), judging from how they look… @nodejs/build?

@jbergstroem
Copy link
Member

@addaleax the /usr/home failures are fixed; it's the result of freebsd symlinking /usr/home/$user to /home/$user and we're making test assumptions against /home/$user.

@addaleax
Copy link
Member Author

addaleax commented Oct 8, 2016

@jbergstroem Thanks for confirming!

In that case, a new CI: https://ci.nodejs.org/job/node-test-commit/5510/

@jbergstroem
Copy link
Member

..apparently not at that host. I'll investigate.

@jbergstroem
Copy link
Member

I've modified HOME for both workers now. Run here: https://ci.nodejs.org/job/node-test-commit-freebsd/4701/nodes=freebsd10-64/console

@addaleax
Copy link
Member Author

addaleax commented Oct 8, 2016

Cool, that leaves only unrelated failures! I’ll land this some time next week unless something comes up.

@jbergstroem
Copy link
Member

Path issues on FreeBSD are fixed.

@rvagg rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:02
Move the internally defined symbol `fs.realpathCacheKey` to
the internal fs module, where it’s more appropriate.

The symbol was recently added in c084287, but since
`internal/fs` is only available in the v7.x branch, this
needs to be a separate follow-up change.

PR-URL: nodejs#8862
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax addaleax force-pushed the fs-internal-realpath-cache-key branch from de961ba to 2bb420b Compare October 26, 2016 20:05
@addaleax
Copy link
Member Author

@jasnell
Copy link
Member

jasnell commented Oct 26, 2016

still LGTM :-)

@addaleax
Copy link
Member Author

addaleax commented Oct 26, 2016

the code didn’t change, I basically just wanted a new CI run :)

edit: sorry, landing this has been laying around in my backlog for a while… let’s start one more fresh CI: https://ci.nodejs.org/job/node-test-commit/6026/

@addaleax
Copy link
Member Author

Landed in 5dea1e2

@addaleax addaleax closed this Nov 16, 2016
@addaleax addaleax deleted the fs-internal-realpath-cache-key branch November 16, 2016 19:31
addaleax added a commit that referenced this pull request Nov 16, 2016
Move the internally defined symbol `fs.realpathCacheKey` to
the internal fs module, where it’s more appropriate.

The symbol was recently added in c084287, but since
`internal/fs` is only available in the v7.x branch, this
needs to be a separate follow-up change.

PR-URL: #8862
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Nov 22, 2016
Move the internally defined symbol `fs.realpathCacheKey` to
the internal fs module, where it’s more appropriate.

The symbol was recently added in c084287, but since
`internal/fs` is only available in the v7.x branch, this
needs to be a separate follow-up change.

PR-URL: #8862
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system. module Issues and PRs related to the module subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants