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

sea: allow requiring core modules with the "node:" prefix #47779

Conversation

RaisinTen
Copy link
Contributor

@RaisinTen RaisinTen commented Apr 29, 2023

Previously, the require() function exposed to the embedded SEA code was calling the internal require() function if the module name belonged to the list of public core modules but the internal require() function does not support loading modules with the "node:" prefix, so this change forwards the calls to another require() function that supports this.

Fixes: nodejs/single-executable#69


bootstrap: fix bug in the snapshot require function and share the code with the SEA require

Previously, requiring node:test while building a snapshot
was failing with a Error: Cannot find module 'node:test'. error, so
this change fixes that and uses the same code path for normalizing ids
for both the snapshot require function as well as the SEA require
function.

Previously:

$ node --snapshot-blob snapshot.blob --build-snapshot <(echo "require('node:test')")
node:internal/main/mksnapshot:105
    throw err;
    ^

Error: Cannot find module 'node:test'.
    at requireForUserSnapshot (node:internal/main/mksnapshot:101:17)
    at /dev/fd/13:1:1
    at main (node:internal/main/mksnapshot:168:5)
    at node:internal/main/mksnapshot:185:1

$ node --snapshot-blob snapshot.blob --build-snapshot <(echo "require('test')")
(node:5389) Warning: built-in module test is not yet supported in user snapshots
(Use `node --trace-warnings ...` to show where the warning was created)
Unknown external reference 0x102d226e0.
<unresolved>
[1]    5389 trace trap  node --snapshot-blob snapshot.blob --build-snapshot <(echo "require('test')")

Now:

$ ./node --snapshot-blob snapshot.blob --build-snapshot <(echo "require('node:test')")
(node:5394) Warning: built-in module node:test is not yet supported in user snapshots
(Use `node --trace-warnings ...` to show where the warning was created)
Unknown external reference 0x105cb25e0.
<unresolved>
[1]    5394 trace trap  ./node --snapshot-blob snapshot.blob --build-snapshot

$ ./node --snapshot-blob snapshot.blob --build-snapshot <(echo "require('test')")
node:internal/main/mksnapshot:106
    throw err;
    ^

Error: Cannot find module 'test'.
    at requireForUserSnapshot (node:internal/main/mksnapshot:102:17)
    at /dev/fd/13:1:1
    at main (node:internal/main/mksnapshot:169:5)
    at node:internal/main/mksnapshot:186:1

Since the Unknown external reference error, which happens now for
the node:test module, was already present for the test module,
it should be okay to fix in a separate PR.

More info about the crash in #47832.

Refs: #47779 (comment)
Signed-off-by: Darshan Sen raisinten@gmail.com


cc @nodejs/single-executable

Previously, the `require()` function exposed to the embedded SEA code
was calling the internal `require()` function if the module name
belonged to the list of public core modules but the internal `require()`
function does not support loading modules with the "node:" prefix, so
this change forwards the calls to another `require()` function that
supports this.

Fixes: nodejs/single-executable#69
Signed-off-by: Darshan Sen <raisinten@gmail.com>
@RaisinTen RaisinTen added the single-executable Issues and PRs related to single-executable applications label Apr 29, 2023
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. labels Apr 29, 2023
@nodejs-github-bot

This comment was marked as outdated.

@RaisinTen RaisinTen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 29, 2023
@nodejs-github-bot

This comment was marked as outdated.

Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should update the documentation to remove the suggestion of Module.createRequire() now that we create one for them?

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@RaisinTen
Copy link
Contributor Author

I think we should update the documentation to remove the suggestion of Module.createRequire() now that we create one for them?

@joyeecheung with this change, the require() function that's exposed to the embedded SEA code is still not fs-based because of the Module.isBuiltin check, so if users need access to the file system, they would still need to use Module.createRequire() to build another require() function.

…e with the SEA require

Previously, this code:
```sh
node --snapshot-blob snapshot.blob --build-snapshot <(echo "require('node:test')")`
```
was failing with a "Error: Cannot find module 'node:test'." error, so
this change fixes that and uses the same code path for normalizing ids
for both the snapshot require function as well as the SEA require
function.

However, building a snapshot when `node:test` is required now fails with:
```sh
./node --snapshot-blob snapshot.blob --build-snapshot <(echo "require('node:test')")
(node:5362) Warning: built-in module node:test is not yet supported in user snapshots
(Use `node --trace-warnings ...` to show where the warning was created)
Unknown external reference 0x10201dd70.
<unresolved>
[1]    5362 trace trap  ./node --snapshot-blob snapshot.blob --build-snapshot
```
which could be solved in a separate PR.

Refs: nodejs#47779 (comment)
Signed-off-by: Darshan Sen <raisinten@gmail.com>
@nodejs-github-bot
Copy link
Collaborator

lib/internal/bootstrap/realm.js Show resolved Hide resolved
@RaisinTen RaisinTen added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. labels May 4, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 4, 2023
@nodejs-github-bot nodejs-github-bot merged commit c868aad into nodejs:main May 4, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in c868aad

@RaisinTen RaisinTen deleted the sea-allow-requiring-core-modules-with-node-prefix branch May 4, 2023 14:33
RaisinTen added a commit to RaisinTen/node that referenced this pull request May 6, 2023
`BuiltinModule.normalizeRequirableId()` was introduced in
nodejs#47779 to fix a bug in the require
function of SEAs and Snapshots, so that built-in modules with the
`node:` scheme could be required correctly. This change makes more use
of this API instead of `BuiltinModule.canBeRequiredByUsers()` and
`BuiltinModule.canBeRequiredWithoutScheme()` to reduce chances of
such bugs.

Signed-off-by: Darshan Sen <raisinten@gmail.com>
nodejs-github-bot pushed a commit that referenced this pull request May 8, 2023
`BuiltinModule.normalizeRequirableId()` was introduced in
#47779 to fix a bug in the require
function of SEAs and Snapshots, so that built-in modules with the
`node:` scheme could be required correctly. This change makes more use
of this API instead of `BuiltinModule.canBeRequiredByUsers()` and
`BuiltinModule.canBeRequiredWithoutScheme()` to reduce chances of
such bugs.

Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: #47896
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
targos pushed a commit that referenced this pull request May 12, 2023
Previously, the `require()` function exposed to the embedded SEA code
was calling the internal `require()` function if the module name
belonged to the list of public core modules but the internal `require()`
function does not support loading modules with the "node:" prefix, so
this change forwards the calls to another `require()` function that
supports this.

Fixes: nodejs/single-executable#69
Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: #47779
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
targos pushed a commit that referenced this pull request May 12, 2023
`BuiltinModule.normalizeRequirableId()` was introduced in
#47779 to fix a bug in the require
function of SEAs and Snapshots, so that built-in modules with the
`node:` scheme could be required correctly. This change makes more use
of this API instead of `BuiltinModule.canBeRequiredByUsers()` and
`BuiltinModule.canBeRequiredWithoutScheme()` to reduce chances of
such bugs.

Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: #47896
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
@danielleadams danielleadams added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Jul 5, 2023
@aduh95
Copy link
Contributor

aduh95 commented Sep 13, 2023

@RaisinTen any chance you would have time to backport this to v18.x?

@RaisinTen
Copy link
Contributor Author

Sure, done - #49648

@RaisinTen RaisinTen added backport-open-v18.x Indicate that the PR has an open backport. and removed backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. labels Sep 14, 2023
targos pushed a commit that referenced this pull request Nov 10, 2023
Previously, the `require()` function exposed to the embedded SEA code
was calling the internal `require()` function if the module name
belonged to the list of public core modules but the internal `require()`
function does not support loading modules with the "node:" prefix, so
this change forwards the calls to another `require()` function that
supports this.

Fixes: nodejs/single-executable#69
Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: #47779
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Signed-off-by: Darshan Sen <raisinten@gmail.com>
@RaisinTen RaisinTen added backported-to-v18.x PRs backported to the v18.x-staging branch. and removed backport-open-v18.x Indicate that the PR has an open backport. labels Nov 10, 2023
targos pushed a commit that referenced this pull request Nov 10, 2023
`BuiltinModule.normalizeRequirableId()` was introduced in
#47779 to fix a bug in the require
function of SEAs and Snapshots, so that built-in modules with the
`node:` scheme could be required correctly. This change makes more use
of this API instead of `BuiltinModule.canBeRequiredByUsers()` and
`BuiltinModule.canBeRequiredWithoutScheme()` to reduce chances of
such bugs.

Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: #47896
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
Previously, the `require()` function exposed to the embedded SEA code
was calling the internal `require()` function if the module name
belonged to the list of public core modules but the internal `require()`
function does not support loading modules with the "node:" prefix, so
this change forwards the calls to another `require()` function that
supports this.

Fixes: nodejs/single-executable#69
Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: nodejs/node#47779
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Signed-off-by: Darshan Sen <raisinten@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
`BuiltinModule.normalizeRequirableId()` was introduced in
nodejs/node#47779 to fix a bug in the require
function of SEAs and Snapshots, so that built-in modules with the
`node:` scheme could be required correctly. This change makes more use
of this API instead of `BuiltinModule.canBeRequiredByUsers()` and
`BuiltinModule.canBeRequiredWithoutScheme()` to reduce chances of
such bugs.

Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: nodejs/node#47896
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
Previously, the `require()` function exposed to the embedded SEA code
was calling the internal `require()` function if the module name
belonged to the list of public core modules but the internal `require()`
function does not support loading modules with the "node:" prefix, so
this change forwards the calls to another `require()` function that
supports this.

Fixes: nodejs/single-executable#69
Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: nodejs/node#47779
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Signed-off-by: Darshan Sen <raisinten@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
`BuiltinModule.normalizeRequirableId()` was introduced in
nodejs/node#47779 to fix a bug in the require
function of SEAs and Snapshots, so that built-in modules with the
`node:` scheme could be required correctly. This change makes more use
of this API instead of `BuiltinModule.canBeRequiredByUsers()` and
`BuiltinModule.canBeRequiredWithoutScheme()` to reduce chances of
such bugs.

Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: nodejs/node#47896
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
abhishekumar-tyagi pushed a commit to abhishekumar-tyagi/node that referenced this pull request May 5, 2024
Previously, the `require()` function exposed to the embedded SEA code
was calling the internal `require()` function if the module name
belonged to the list of public core modules but the internal `require()`
function does not support loading modules with the "node:" prefix, so
this change forwards the calls to another `require()` function that
supports this.

Fixes: nodejs/single-executable#69
Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: nodejs/node#47779
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Signed-off-by: Darshan Sen <raisinten@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backported-to-v18.x PRs backported to the v18.x-staging branch. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. single-executable Issues and PRs related to single-executable applications util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing internal module