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

chore: backport a536de469 from node #34136

Merged
merged 1 commit into from May 11, 2022
Merged

Conversation

deepak1556
Copy link
Member

Description of Change

Backports nodejs/node@a536de4

Fixes 32-bit native module builds with node-gyp@9.0.0 that uses config.gypi from bundled custom headers. Also removes redundant patch enable_31_bit_smis_on_64bit_arch_and_ptr_compression.patch since we always ship a single copy of config.gypi with pointer compression enabled since d1e0b63

Release Notes

Notes: Fix 32-bit native module builds with node-gyp >= 9.0.0

@deepak1556 deepak1556 requested review from a team as code owners May 8, 2022 17:03
@deepak1556 deepak1556 requested a review from zcbenz May 8, 2022 17:03
@deepak1556 deepak1556 added semver/patch backwards-compatible bug fixes backport-check-skip Skip trop's backport validity checking 17-x-y target/16-x-y labels May 8, 2022
@deepak1556
Copy link
Member Author

nan tests are failing because of older node-gyp toolchain being used which does not respect config.gypi generated by us.

gyp info using node-gyp@7.1.2
gyp info using node@14.17.5 | linux | x64

@MarshallOfSound I would like to remove enable_31_bit_smis_on_64bit_arch_and_ptr_compression.patch as follow-up on main and other branches as well, can we bump the node version for CI to use latest LTS node ? Another option, is we can bump the node-gyp version for the nan tests at

cd src
node electron/script/nan-spec-runner.js
, which path is preferred ?

@jkleinsc
Copy link
Contributor

jkleinsc commented May 9, 2022

@deepak1556 we should upgrade node in CI. This will require a new docker image. I started a PR here: electron/build-images#26

@deepak1556
Copy link
Member Author

Great, thanks!

@deepak1556
Copy link
Member Author

@jkleinsc 17-x-y is on an older image than main, 19-x-y and 18-x-y,

I don't think we can make a image update for this branch, so looks like updating node-gyp as part of the nan step would be better for this branch and 16-x-y ?

As for the newer branches, will they automatically get the node 16 image from electron/build-images#26 ? or does this require a tag update on electron/electron end similar to #27915 ?

@deepak1556 deepak1556 force-pushed the robo/fix_gypi_for_32bit_17_x_y branch from e4c3e6d to 9e4965f Compare May 10, 2022 15:30
@deepak1556
Copy link
Member Author

Abandoned removing enable_31_bit_smis_on_64bit_arch_and_ptr_compression.patch for this branch, since it requires changes to CI images. PR now only backports the upstream change required to address 32-bit build failures.

@deepak1556 deepak1556 merged commit 16b78a4 into 17-x-y May 11, 2022
@deepak1556 deepak1556 deleted the robo/fix_gypi_for_32bit_17_x_y branch May 11, 2022 00:26
@release-clerk
Copy link

release-clerk bot commented May 11, 2022

Release Notes Persisted

Fix 32-bit native module builds with node-gyp >= 9.0.0

@trop
Copy link
Contributor

trop bot commented May 11, 2022

I have automatically backported this PR to "16-x-y", please check out #34167

@jkleinsc
Copy link
Contributor

@deepak1556 it would require updating the CircleCI config to pull in the right image by changing https://github.com/electron/electron/blob/main/.circleci/config/base.yml#L47

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
17-x-y backport-check-skip Skip trop's backport validity checking semver/patch backwards-compatible bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants