FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

deps: allow amaro to be externalizable by mhdawson · Pull Request #54646 · nodejs/node · GitHub

/ node Public

deps: allow amaro to be externalizable - #54646

Closed
mhdawson wants to merge 5 commits into
nodejs:mainfrom
mhdawson:support-amaro-externalization
ConversationCommits (5)ChecksFiles changed
Closed

deps: allow amaro to be externalizable#54646
mhdawson wants to merge 5 commits into
nodejs:mainfrom
mhdawson:support-amaro-externalization

Conversation

mhdawson commented Aug 29, 2024

Copy link
Copy Markdown
Member

deps: allow amaro to be externalizable
a5a4049 
- allow amaro to be externalized like other builtins
  containing WASM. More context is available in
  https://github.com/nodejs/node/blob/main/doc/contributing/maintaining/maintaining-dependencies.md#supporting-externalizable-dependencies-with-javascript-code

Signed-off-by: Michael Dawson <midawson@redhat.com>

nodejs-github-bot commented Aug 29, 2024

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/startup

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 commented Aug 29, 2024

Copy link
Copy Markdown
Member Author

FYI @marco-ippolito

avivkeller added the strip-types Issues or PRs related to strip-types support label Aug 29, 2024

codecov Bot commented Aug 29, 2024 •
edited
Loading

Copy link
Copy Markdown

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 added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 30, 2024
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 commented Aug 30, 2024

Copy link
Copy Markdown
Collaborator

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

richardlau commented Aug 30, 2024

Copy link
Copy Markdown
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 approved these changes Aug 31, 2024
View reviewed changes
Comment thread src/node_builtins.cc Outdated
mhdawson and others added 2 commits September 3, 2024 11:01
Update src/node_builtins.cc
a276828 
Co-authored-by: Luigi Pinca <luigipinca@gmail.com>
squash: address review comments
70154e9 
Signed-off-by: Michael Dawson <midawson@redhat.com>

mhdawson commented Sep 3, 2024

Copy link
Copy Markdown
Member Author

@richardlau added the guard for process.versions.

mhdawson added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 3, 2024
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 commented Sep 3, 2024

Copy link
Copy Markdown
Collaborator

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

legendecas reviewed Sep 3, 2024
View reviewed changes
Comment thread src/node_metadata.cc
squash: address comments
3e9f641 
Signed-off-by: Michael Dawson <midawson@redhat.com>
mhdawson added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 3, 2024
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 commented Sep 3, 2024

Copy link
Copy Markdown
Collaborator

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

legendecas reviewed Sep 3, 2024
View reviewed changes
Comment thread src/node_metadata.h
Update src/node_metadata.h
865ac33 
Co-authored-by: Chengzhong Wu <legendecas@gmail.com>
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 added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 4, 2024
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 commented Sep 4, 2024

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Sep 5, 2024

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Sep 5, 2024

Copy link
Copy Markdown
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
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 <midawson@redhat.com>
mhdawson mentioned this pull request Sep 5, 2024
Closed
mhdawson added a commit that referenced this pull request Sep 5, 2024
deps: allow amaro to be externalizable
ce19715 
- allow amaro to be externalized like other builtins
  containing WASM. More context is available in
  https://github.com/nodejs/node/blob/main/doc/contributing/maintaining/maintaining-dependencies.md#supporting-externalizable-dependencies-with-javascript-code

Signed-off-by: Michael Dawson <midawson@redhat.com>
PR-URL: #54646
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

mhdawson commented Sep 5, 2024

Copy link
Copy Markdown
Member Author

Landed in ce19715

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

Signed-off-by: Michael Dawson <midawson@redhat.com>
PR-URL: #54646
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
RafaelGSS mentioned this pull request Sep 16, 2024
Merged
mhdawson added a commit that referenced this pull request Sep 16, 2024
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 <midawson@redhat.com>
PR-URL: #54792
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com>
targos pushed a commit that referenced this pull request Oct 4, 2024
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 <midawson@redhat.com>
PR-URL: #54792
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com>
marco-ippolito pushed a commit that referenced this pull request Nov 16, 2024
doc: update documentation for externalizing deps
dfd0318 
Refs: #54646

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

Signed-off-by: Michael Dawson <midawson@redhat.com>
PR-URL: #54792
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com>
marco-ippolito pushed a commit that referenced this pull request Nov 17, 2024
doc: update documentation for externalizing deps
a4586f0 
Refs: #54646

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

Signed-off-by: Michael Dawson <midawson@redhat.com>
PR-URL: #54792
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Ulises Gascón <ulisesgascongonzalez@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
lpinca lpinca approved these changes
richardlau richardlau approved these changes
legendecas legendecas approved these changes
marco-ippolito marco-ippolito approved these changes
+1 more reviewer
aymen94 aymen94 approved these changes
Reviewers whose approvals may not affect merge requirements

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. 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

Back | FazBrowse Home | New Git URL