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

feat: support native builds with Node 20 #771

Merged
merged 12 commits into from
Aug 2, 2023
Merged

feat: support native builds with Node 20 #771

merged 12 commits into from
Aug 2, 2023

Conversation

seemk
Copy link
Contributor

@seemk seemk commented Aug 2, 2023

And add Node 20 to testing matrix.

@seemk seemk requested review from a team as code owners August 2, 2023 09:50
@codecov-commenter
Copy link

Codecov Report

Patch and project coverage have no change.

Comparison is base (f1f0f32) 86.10% compared to head (ac09699) 86.10%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #771   +/-   ##
=======================================
  Coverage   86.10%   86.10%           
=======================================
  Files          19       19           
  Lines         662      662           
  Branches      151      151           
=======================================
  Hits          570      570           
  Misses         92       92           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@seemk seemk merged commit 8cac4f7 into main Aug 2, 2023
@seemk seemk deleted the node20 branch August 2, 2023 09:59
@seemk seemk mentioned this pull request Aug 2, 2023
@lbeschastny
Copy link

lbeschastny commented Aug 2, 2023

@seemk any chance you could add those changes to 1.x branch as well?

It looks like it should be enough to just cherry-pick 8cac4f7 merge commit there:

git cherry-pick -m 1 8cac4f7

Update:

There are some conflicts

	both modified:   .github/workflows/ci.yml
	both modified:   scripts/prebuild-os.js

but they looks simple enough to fix.

lbeschastny pushed a commit to lbeschastny/splunk-otel-js that referenced this pull request Aug 2, 2023
* node20 build attempt

* enable c++17

* suppress macos warnings

* prebuild on node 16

* conditional cpp version

* fix conditional

* build node20 on node16

* use cppstd14 on macos

* binding gyp syntax

* remove unnecessary quotes

* set cpp version on windows

* always use cpp17 on macos
@lbeschastny
Copy link

lbeschastny commented Aug 2, 2023

Looks like it's not that straightforward. The build in 1.x branch didn't work after cherry-picking the changes from the main branch.

And I'm not sure if it's worth investigating and fixing.

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.

4 participants