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.

Sign up for GitHub

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

Jump to bottom

deps: allow amaro to be externalizable #54646

Closed
wants to merge 5 commits into from
Closed

deps: allow amaro to be externalizable #54646

wants to merge 5 commits into from

Conversation

mhdawson
Copy link
Member

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/startup

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Aug 29, 2024
@mhdawson
Copy link
Member Author

FYI @marco-ippolito

@RedYetiDev RedYetiDev added the strip-types Issues or PRs related to strip-types support label Aug 29, 2024
@codecov Codecov
Copy link

codecov bot commented Aug 29, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.61%. Comparing base (fc02b88) to head (865ac33).
Report is 283 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #54646      +/-   ##
==========================================
+ Coverage   87.30%   87.61%   +0.30%     
==========================================
  Files         649      650       +1     
  Lines      182755   182829      +74     
  Branches    35044    35384     +340     
==========================================
+ Hits       159552   160181     +629     
+ Misses      16466    15919     -547     
+ Partials     6737     6729       -8
Files with missing lines Coverage Δ
src/node_builtins.cc 78.43% <ø> (-0.24%) ⬇️
src/node_metadata.cc 91.66% <ø> (ø)

... and 74 files with indirect coverage changes

@mhdawson mhdawson added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 30, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 30, 2024
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/61708/

@richardlau
Copy link
Member

FWIW process.versions is wrong if using externalized amaro -- it will print 0.1.8 from

node/src/amaro_version.h

Line 5 in 5a22d8e

#define AMARO_VERSION "0.1.8"
regardless of whatever version the externalized amaro is. Compare with, e.g. undici where if an externalized version is used it is omitted from process.versions:

node/src/node_metadata.cc

Lines 122 to 124 in 5a22d8e

#ifndef NODE_SHARED_BUILTIN_UNDICI_UNDICI_PATH
undici = UNDICI_VERSION;
#endif

lpinca
lpinca approved these changes Aug 31, 2024
View reviewed changes
src/node_builtins.cc Outdated Show resolved Hide resolved
mhdawson and others added 2 commits September 3, 2024 11:01
@mhdawson @lpinca
Update src/node_builtins.cc
a276828 
Co-authored-by: Luigi Pinca <[email protected]>
@mhdawson
squash: address review comments
70154e9 
Signed-off-by: Michael Dawson <[email protected]>
@mhdawson
Copy link
Member Author

mhdawson commented Sep 3, 2024

@richardlau added the guard for process.versions.

@mhdawson mhdawson added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 3, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 3, 2024
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/61865/

@mhdawson
squash: address comments
3e9f641 
Signed-off-by: Michael Dawson <[email protected]>
@mhdawson mhdawson added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 3, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 3, 2024
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/61880/

legendecas
legendecas reviewed Sep 3, 2024
View reviewed changes
src/node_metadata.h Outdated Show resolved Hide resolved
@mhdawson @legendecas
Update src/node_metadata.h
865ac33 
Co-authored-by: Chengzhong Wu <[email protected]>
@richardlau richardlau added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Sep 4, 2024
@mhdawson mhdawson added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 4, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 4, 2024
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/61920/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/61960/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/62000/

mhdawson added a commit to mhdawson/io.js that referenced this pull request Sep 5, 2024
@mhdawson
doc: update documentation for externalizing deps
de1e9a7 
Refs: nodejs#54646

- Add instructions to update how process.versions is reported
  as I missed that in a recent addition.

Signed-off-by: Michael Dawson <[email protected]>
@mhdawson mhdawson mentioned this pull request Sep 5, 2024
Closed
mhdawson added a commit that referenced this pull request Sep 5, 2024
@mhdawson
deps: allow amaro to be externalizable
ce19715 
- allow amaro to be externalized like other builtins
  containing WASM. More context is available in
  https:/nodejs/node/blob/main/doc/contributing/maintaining/maintaining-dependencies.md#supporting-externalizable-dependencies-with-javascript-code

Signed-off-by: Michael Dawson <[email protected]>
PR-URL: #54646
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@mhdawson
Copy link
Member Author

mhdawson commented Sep 5, 2024

Landed in ce19715

@mhdawson mhdawson closed this Sep 5, 2024
aduh95 pushed a commit that referenced this pull request Sep 12, 2024
@mhdawson @aduh95
deps: allow amaro to be externalizable
0659516 
- allow amaro to be externalized like other builtins
  containing WASM. More context is available in
  https:/nodejs/node/blob/main/doc/contributing/maintaining/maintaining-dependencies.md#supporting-externalizable-dependencies-with-javascript-code

Signed-off-by: Michael Dawson <[email protected]>
PR-URL: #54646
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Sep 16, 2024
Merged
mhdawson added a commit that referenced this pull request Sep 16, 2024
@mhdawson
doc: update documentation for externalizing deps
7feff24 
Refs: #54646

- Add instructions to update how process.versions is reported
  as I missed that in a recent addition.

Signed-off-by: Michael Dawson <[email protected]>
PR-URL: #54792
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Akhil Marsonya <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Ulises Gascón <[email protected]>
@targos targos added the dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label Sep 22, 2024
targos pushed a commit that referenced this pull request Oct 4, 2024
@mhdawson @targos
doc: update documentation for externalizing deps
c1d73ab 
Refs: #54646

- Add instructions to update how process.versions is reported
  as I missed that in a recent addition.

Signed-off-by: Michael Dawson <[email protected]>
PR-URL: #54792
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Akhil Marsonya <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Ulises Gascón <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@lpinca lpinca lpinca approved these changes

@richardlau richardlau richardlau approved these changes

@legendecas legendecas legendecas approved these changes

@aymen94 aymen94 aymen94 approved these changes

@marco-ippolito marco-ippolito marco-ippolito approved these changes

Assignees
No one assigned
Labels
commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. strip-types Issues or PRs related to strip-types support
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@mhdawson @nodejs-github-bot @richardlau @jasnell @lpinca @legendecas @aymen94 @marco-ippolito @targos @RedYetiDev