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

build: enable loading builtin modules at runtime #36215

Closed
RaisinTen opened this issue Nov 21, 2020 · 14 comments
Closed

build: enable loading builtin modules at runtime #36215

RaisinTen opened this issue Nov 21, 2020 · 14 comments
Labels
build Issues and PRs related to build files or the CI.

Comments

@RaisinTen
Copy link
Contributor

Is your feature request related to a problem? Please describe.
It might be a problem to build node entirely every time the path to the node builtin modules is changed.

Describe the solution you'd like

  • During the configuration stage, instead of using --node-builtin-modules-path with a path value, we can use an --enable-runtime-node-builtin-modules flag to enable this feature.
  • At runtime, the path to the builtin modules can be read in from an environment variable NODE_BUILTIN_MODULES_PATH using libuv OR it can be passed to node as a command line argument using --node-builtin-modules-path with a path value.

Fixes: nodejs/help#3087
Ref: #31321

@richardlau
Copy link
Member

I’m very hesitant to make this a runtime option. It’s fine as a build time option for development of Node.js core but opens up potential attack vectors to replace the built-in modules. The work towards more use of primordials in the built-in modules suggests we are moving away from allowing the built-ins from being changed at runtime.

@devsnek
Copy link
Member

devsnek commented Nov 21, 2020

-1 on this. I made the configure option as dumb as possible so people don't get ideas about using it outside of quick development changes to node's builtin js files.

@RaisinTen
Copy link
Contributor Author

This is only for quick development changes to node's builtin js files. What's wrong with having this as a runtime option? How do you attack something which is not even compiled because it has been excluded because of guarding ifdefs?

@devsnek
Copy link
Member

devsnek commented Nov 21, 2020

I don't understand what the point of this is. What functionality is --node-builtin-modules-path=$(cwd) missing?

@RaisinTen
Copy link
Contributor Author

It's hard to change the location frequently. Reading the discussion in the PR, it felt as if this was intended to be made into a runtime feature but it didn't happen because you weren't sure about how to fetch the data from the options parser. I don't quite see what prevents us from making this flexible given that people won't be using it outside of development anyway.

@devsnek
Copy link
Member

devsnek commented Nov 22, 2020

Why does the location need to change? I'm curious about the use case which is driving this issue.

@RaisinTen
Copy link
Contributor Author

The location changes if:

  • the same executable is used to load the builtin modules from different directories without having to build node in all of them - this makes it easier to compare js changes
  • the node directory is moved around

@devsnek
Copy link
Member

devsnek commented Nov 22, 2020

if you have separate node checkouts imo you should just build node once in each checkout.

@RaisinTen
Copy link
Contributor Author

That takes more time. Instead if we just keep the other repos updated and build/rebuild node in one, it's way faster to develop.

@mmomtchev
Copy link
Contributor

By the way, there is support for that option coming in the November edition of the JS debugger
microsoft/vscode-js-debug@737f6c0

The vscode-js-debug has been rather nice, because this is a specific feature for Node developers

@RaisinTen
Copy link
Contributor Author

That's very cool! I don't use vscode unfortunately. :/

@PoojaDurgad PoojaDurgad added the build Issues and PRs related to build files or the CI. label Dec 16, 2020
@Melab
Copy link

Melab commented Oct 24, 2021

This should happen. It should also be such that files can be added and removed from the set of builtin modules.

@Melab
Copy link

Melab commented Oct 24, 2021

if you have separate node checkouts imo you should just build node once in each checkout.

But that takes much time. I've had faster build times with the Linux kernel than with V8 (which is just a small part of Node) and changing small amounts of code that could be modular for testing and development purposes makes development take orders of magnitude longer.

@RaisinTen
Copy link
Contributor Author

I was using a slow setup (a very old 32-bit HP Pavilion running Ubuntu 18) when I posted this issue. This is not a problem for me anymore now that I'm on an M1 MBP, closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI.
Projects
None yet
Development

No branches or pull requests

6 participants