node-gyp icon indicating copy to clipboard operation
node-gyp copied to clipboard

fix: create Python symlink only during builds, and clean it up after

Open pimterry opened this issue 3 years ago • 2 comments

Checklist
  • [x] npm install && npm test passes
  • [ ] 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.

pimterry avatar Aug 22 '22 15:08 pimterry

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 avatar Sep 19 '22 08:09 cclauss

@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 avatar Sep 19 '22 11:09 pimterry

@pimterry @cclauss This is still causing problems for a lot of people. Is this fix going to be merged soon?

apaprocki avatar Nov 11 '22 16:11 apaprocki

Any chance of getting this merged?

jagthedrummer avatar Feb 04 '23 15:02 jagthedrummer

Please rebase to fix git conflicts.

cclauss avatar Jul 17 '23 08:07 cclauss

Please rebase to fix git conflicts.

@cclauss done - the build is just waiting approval

pimterry avatar Jul 17 '23 10:07 pimterry

@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

cclauss avatar Jul 17 '23 11:07 cclauss

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

pimterry avatar Jul 17 '23 11:07 pimterry

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.

willm avatar Aug 01 '23 05:08 willm

The failing Visual Studio tests are fixed elsewhere.

cclauss avatar Aug 01 '23 08:08 cclauss

Thank you @cclauss 🙇

brendonjohn avatar Aug 02 '23 23:08 brendonjohn