-
Notifications
You must be signed in to change notification settings - Fork 30k
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@nodejs/build … quirky CI failure? https://ci.nodejs.org/job/node-test-commit-arm/5398/nodes=ubuntu1604-arm64/console |
@addaleax .. yeah, I've been spotting that one also. appears unrelated to this change. |
So… trying again: |
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, |
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. |
The issues in #8949 are different, unfortunately.
I’d agree, this patch can’t really cause them. But since it looks like the failures are 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? |
@addaleax the |
@jbergstroem Thanks for confirming! In that case, a new CI: https://ci.nodejs.org/job/node-test-commit/5510/ |
..apparently not at that host. I'll investigate. |
I've modified HOME for both workers now. Run here: https://ci.nodejs.org/job/node-test-commit-freebsd/4701/nodes=freebsd10-64/console |
Cool, that leaves only unrelated failures! I’ll land this some time next week unless something comes up. |
Path issues on FreeBSD are fixed. |
c133999
to
83c7a88
Compare
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>
de961ba
to
2bb420b
Compare
rebased, new CI: https://ci.nodejs.org/job/node-test-pull-request/4693/ |
still LGTM :-) |
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/ |
Landed in 5dea1e2 |
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>
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>
Checklist
make -j8 test
(UNIX), orvcbuild test nosign
(Windows) passesAffected 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.