Skip to content
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

fix: ignore node_gyp_bins if it exists #1391

Merged
merged 2 commits into from
Aug 11, 2022

Conversation

VerteDinde
Copy link
Member

@VerteDinde VerteDinde commented Aug 2, 2022

  • I have read the contribution documentation for this project.
  • I agree to follow the code of conduct that this project follows, as appropriate.
  • The changes are appropriately documented (if applicable).
  • The changes have sufficient test coverage (if applicable).
  • The testsuite passes successfully on my local machine (if applicable). (Passes on MacOS Monterey x64 & arm64)

Addresses electron/rebuild#1024

In node-gyp v9.0.0, node-gyp adds a python symlink for older versions of python. This symlink is added to a ${module_name}/build/node_gyp_bins/ directory within the build directory, which electron-rebuild then tries to rebuild.

This PR ignores the node_gyp_bins directory, and adds tests for making sure that the directory is not added to the packaged app.

@welcome
Copy link

welcome bot commented Aug 2, 2022

Thanks for opening a pull request!

Here are some highlighted action items that will help get it across the finish line, from the
pull request guidelines:

  • Follow the JavaScript coding style.
  • Run npm run lint locally to catch formatting errors earlier.
  • Document any user-facing changes in NEWS.md and other docs.
  • Include tests when adding/changing behavior.

Development and triage is community-driven, so please be patient and we will get back to you as soon as we can.

hash: '27f2dba4273f6c119000ec7059c27d86e27306d5dbbb83cfdfb862d92c679574'
hash: '2ef4c03f6fc8adf03f782b40fddcfcdd26d1f3f3669ecedffeea5de5d28897c3'
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm oretty sure that this hash changed because I updated the basic app that baseOpts is passing into the test. Tested this locally on an x64 machine, and the hash is consistently returning this value both locally and in CI. However, if this doesn't seem to pass the sniff test, let me know, happy to take another look.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm oretty sure that this hash changed because I updated the basic app that baseOpts is passing into the test.

Might want to add a comment to this effect for future folks (and also how to find out the new value).

@VerteDinde VerteDinde marked this pull request as ready for review August 2, 2022 04:08
@malept
Copy link
Member

malept commented Aug 3, 2022

Hmmm. Separate PR to fix the xcode version in use, so the tests pass?

@VerteDinde
Copy link
Member Author

@malept Yeah, hilariously that image failure is new as of this afternoon's rerun 😄 I'll fix that in another PR and then rebase

Copy link
Member

@malept malept left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside from a maintenance comment, this seems fine.

hash: '27f2dba4273f6c119000ec7059c27d86e27306d5dbbb83cfdfb862d92c679574'
hash: '2ef4c03f6fc8adf03f782b40fddcfcdd26d1f3f3669ecedffeea5de5d28897c3'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm oretty sure that this hash changed because I updated the basic app that baseOpts is passing into the test.

Might want to add a comment to this effect for future folks (and also how to find out the new value).

@codecov
Copy link

codecov bot commented Aug 11, 2022

Codecov Report

Merging #1391 (7f1bc03) into main (c6b4c78) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1391   +/-   ##
=======================================
  Coverage   95.61%   95.61%           
=======================================
  Files          15       15           
  Lines         776      776           
=======================================
  Hits          742      742           
  Misses         34       34           
Impacted Files Coverage Δ
src/copy-filter.js 100.00% <ø> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants