-
Notifications
You must be signed in to change notification settings - Fork 30.7k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
build: prevent C++ linker from overwriting executable #22027
Conversation
@nodejs/build-files |
Are we sure it's only a Mac thing? I think I saw something similar on Linux when I was working on canary. I fixed it with |
I'm -0 on this. IMHO it just adds complexity to workaround the crux of the issue. |
IIRC, this is not mac-only but makefile-only - we have been seeing the executable being deleted back in the days when the addons were built in a loop in makefile instead of in the new JS scripts. |
Resume Build: https://ci.nodejs.org/job/node-test-pull-request/16095/ |
Oh, wait, we need an entire new CI since there were changes since the last one. |
d8101b2
to
cf4a07a
Compare
0eaaa03
to
0437cef
Compare
Running `make run-ci` with multiple jobs, it is possible for the C++ linker to be invoked while build-addons is running, this can result in intermittent ENOENT failures when spawning `nodeGyp`. To prevent this, originally provide a different name for the executable, and rename it to the correct name only after the link completes successfully. Fixes 22006. Kinda. Sorta.
Rebased and force-pushed to fix the CI problem (unrelated to this PR). |
@nodejs/gyp @nodejs/node-gyp Would be great to get a second review on this one. Aside: Should we really have separate |
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.
I agree with @refack, this is more of a workaround than a fix.
@@ -925,6 +931,25 @@ | |||
}, | |||
], | |||
} ], | |||
# When building executable on Unix operating systems, in order to | |||
# avoid overwriting the executable while it it running, build it with |
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.
typo: is
Even though this is clearly a workaround: is there any solution in sight for the actual issue? If not, I do not see any downside in doing this for now. Any other options about that? |
With two approvals, I will merge this (after fixing the typo) on, say, ... Friday? ...unless there are any objections. |
Closing this due to inactivity. Pleas re-open if needed. |
Running
make run-ci
with multiple jobs, it is possible for the C++ linkerto be invoked while build-addons is running, this can result in intermittent
ENOENT failures when spawning
nodeGyp
. To prevent this, originallyprovide a different name for the executable, and rename it to the correct
name only after the link completes successfully.
Fixes #22006. Kinda. Sorta.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes