GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — deps: allow amaro to be externalizable by mhdawson · Pull Request #54646 · nodejs/node

Conversation

@mhdawson

@mhdawson mhdawson commented Aug 29, 2024

Copy link
Copy Markdown
Member
  • 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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bricss reacted with rocket emoji
All reactions
  • 🚀 1 reaction
@mhdawson
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 <[email protected]>
@nodejs-github-bot

nodejs-github-bot commented Aug 29, 2024

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/startup
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels Aug 29, 2024
@mhdawson

mhdawson commented Aug 29, 2024

Copy link
Copy Markdown
Member Author

FYI @marco-ippolito

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@avivkeller avivkeller added the strip-types Issues and PRs related to TypeScript type stripping. label Aug 29, 2024
@codecov

codecov Bot commented Aug 29, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

marco-ippolito
marco-ippolito approved these changes Aug 30, 2024
View reviewed changes
@mhdawson mhdawson added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 30, 2024
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 30, 2024
@nodejs-github-bot

nodejs-github-bot commented Aug 30, 2024

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

aymen94
aymen94 approved these changes Aug 30, 2024
View reviewed changes
richardlau
richardlau approved these changes Aug 30, 2024
View reviewed changes
@richardlau

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

legendecas reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

lpinca
lpinca approved these changes Aug 31, 2024
View reviewed changes
Comment thread src/node_builtins.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.

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

mhdawson commented Sep 3, 2024

Copy link
Copy Markdown
Member Author

@richardlau added the guard for process.versions.

richardlau reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mhdawson mhdawson added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 3, 2024
richardlau
richardlau approved these changes Sep 3, 2024
View reviewed changes
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 3, 2024
@nodejs-github-bot

nodejs-github-bot commented Sep 3, 2024

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

legendecas
legendecas reviewed Sep 3, 2024
View reviewed changes
Comment thread src/node_metadata.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.

@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. Only starts once the PR has an approving review. label Sep 3, 2024
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 3, 2024
@nodejs-github-bot

nodejs-github-bot commented Sep 3, 2024

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

legendecas
legendecas reviewed Sep 3, 2024
View reviewed changes
Comment thread src/node_metadata.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

@mhdawson @legendecas
Update src/node_metadata.h
865ac33
Co-authored-by: Chengzhong Wu <[email protected]>
@richardlau richardlau added the commit-queue-squash PRs the Commit Queue should land as one squashed commit. label Sep 4, 2024
@mhdawson mhdawson added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 4, 2024
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 4, 2024
@nodejs-github-bot

nodejs-github-bot commented Sep 4, 2024

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

legendecas
legendecas approved these changes Sep 4, 2024
View reviewed changes
@nodejs-github-bot

nodejs-github-bot commented Sep 5, 2024

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@nodejs-github-bot

nodejs-github-bot commented Sep 5, 2024

Copy link
Copy Markdown
Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jasnell
jasnell approved these changes Sep 5, 2024
View reviewed changes
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
doc: update documentation for externalizing deps #54792
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://github.com/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

mhdawson commented Sep 5, 2024

Copy link
Copy Markdown
Member Author

Landed in ce19715

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@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://github.com/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
v22.9.0 proposal #54966
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 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]>
marco-ippolito pushed a commit that referenced this pull request Nov 16, 2024
@mhdawson @marco-ippolito
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 <[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]>
marco-ippolito pushed a commit that referenced this pull request Nov 17, 2024
@mhdawson @marco-ippolito
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 <[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
@marco-ippolito marco-ippolito marco-ippolito approved these changes
+1 more reviewer
@aymen94 aymen94 aymen94 approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

commit-queue-squash PRs the Commit Queue should land as one squashed commit. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. strip-types Issues and PRs related to TypeScript type stripping.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

10 participants

@mhdawson @nodejs-github-bot @richardlau @jasnell @lpinca @legendecas @aymen94 @marco-ippolito @targos @avivkeller
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.