fix: create Python symlink only during builds, and clean it up after
Checklist
- [x]
npm install && npm testpasses - [ ] tests are included
- [ ] documentation is changed or added
- [x] commit message follows commit guidelines
Not 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 b9ddcd5bbd93b05b03674836b6ebdae2c2e74c8c 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 codesign tool) 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.
There are 14 https://github.com/nodejs/node-gyp/labels/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 node_gyp_bins symlink directory).
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.
@pimterry @cclauss This is still causing problems for a lot of people. Is this fix going to be merged soon?
Any chance of getting this merged?
Please rebase to fix git conflicts.
Please rebase to fix git conflicts.
@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: https://github.com/nodejs/node-gyp/pull/2886/files
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 🙇