fix: create Python symlink only during builds, and clean it up after #2721
Conversation
|
There are 14 ffi-napi issues on this repo. Could this be a solution to those issues? |
|
@cclauss #2713 yes, but not the others I think (#2612 is conceptually similar, but I think technically unrelated - this PR only affects the Although #2713 is fixed by this and does reference ffi-napi as an example, it is a general issue. I expect many of these issues are only reported for ffi-napi because it's a very common non-trivial native module, rather than because they're all necessarily related. |
There was a problem hiding this comment.
I happen to like this PR, since it resolves a user concern for a feature I wrote in #2362.
The issue is apparently big enough to prevent people from publishing their app to app stores if node-gyp has been used to build code anywhere in their app.
The approach taken is one of the ones I suggested in my original PR.
With this PR, node-gyp records the validated python path (from lib/find-python.js) in the generated config.gypi, and reads that at build time to symlink to the validated python bin for the build. This means we can delete the symlink when the build finishes, rather than having to leave it to accommodate users who manually run node-gyp configure then separately node-gyp build.
I'd like to see this merged, personally speaking.
| @@ -75,6 +75,9 @@ async function getCurrentConfigGypi ({ gyp, nodeDir, vsInfo }) { | |||
| // set the node development directory | |||
| variables.nodedir = nodeDir | |||
|
|
|||
| // set the configured Python path | |||
| variables.python = python | |||
There was a problem hiding this comment.
My only concern is, if there are any existing packages which end up setting a variable variables.python in their config.gypi?
Maybe this should be something more specific where package authors would really have to try to collide with node-gyp's namespace, like variables.node_gyp.python_path or variables.node_gyp_python_path?
(I'm admittedly not a heavy node-gyp or gyp user, so maybe someone would know if that's a realistic concern or not?)
NOTE: I am not a maintainer for this repository, (just a past contributor who wrote the feature this PR is fixing unintended side effects of), so maintainers here have final say and this is just a personal opinion of someone technically outside the project. Either way, I'd like to see this get merged soon, if possible.
|
Any chance of getting this merged? |
There was a problem hiding this comment.
looks good! Nice job w/ the node14 compat and isolating the symlinking behavior to lib/build.js (with appropriate cleanup).
If possible, a nicer solution would be to directly use the config.variables.python as needed rather than symlinks to the build bins dir - but that feels much larger scope than this fix. This fix feels like an improvement.
Regarding testing, maybe a few manual tests to show it works would mitigate concern - making sure it works on node 12/14/16/18 and using find . -type l python3 to make sure nothing ends up there.
|
Please rebase to fix git conflicts. |
Previously in b9ddcd5 this was created during configuration, and the symlink persisted indefinitely. This causes problems with many tools that do not expect a codebase to include symlinks to external absolute paths. This PR largely reverts that commit, and instead writes the path to link to into the config, and then creates the symlink only temporarily during the build process, always deleting it afterwards.
pimterry
force-pushed
the
cleanup-symlinks
branch
from
fc0a0bf
to
eb50fe0
Compare
@cclauss done - the build is just waiting approval |
|
@StefanStojanovic @lukekarrys Your reviews, please. The Python lint error can be ignored because it is fixed at: |
|
The visual-studio error (a test after-all timeout) that's failing here seems ignorable too - that's been failing in other unrelated changes already, including this docs-only PR: https://github.com/nodejs/node-gyp/actions/runs/5412919694/jobs/10135897829 |
|
Also keen for this to get merged. FYI the python lint error that is failing has been fixed upstream, can this be rebased? Maybe it has more chance of getting merged if the status checks pass. |
|
The failing Visual Studio tests are fixed elsewhere. |
|
Thank you @cclauss 🙇 |
Checklist
npm install && npm testpassesNot really sure how to automatically test this fix, since the whole idea is that the symlink is leaves no trace, and I don't think there's any documentation required here.
Description of change
Fixes #2713
Previously in b9ddcd5 this python symlink was created during configuration, and the symlink persisted indefinitely. This causes problems with many tools (including Electron Builder, Electron Forge, and Apple's
codesigntool) that do not expect a codebase to ever include symlinks to external absolute paths.This PR largely reverts that commit, and instead just writes the path to link to into the config, and then creates the symlink only temporarily during the build process, always deleting it afterwards.