-
Notifications
You must be signed in to change notification settings - Fork 30k
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 internal modules from disk #31321
build: enable loading internal modules from disk #31321
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still sad that this isn’t a runtime option, but thanks for picking this up again.
@addaleax I'd be happy to make it a runtime option. I'm not sure how I'd get data from the options parser to the internal module loader though. |
This is great! I am fine with it being a configure option (maybe just for now), given that we probably wouldn't want people to use it outside of development. (For example, the current PR seems to resolve the path relative to the current working directory, which seems dangerous outside of the repo.) |
Should this print a warning telling end users not to use it? |
+1 to emitting a warning on this. |
I think if we turn this into a runtime option, the warning may be appropriate, but for a compile time option I don’t quite see the point – whoever built the Node.js binary explicitly opted into a developer feature, and adding the warning is going to make running the test suite infeasible, which makes it less useful as a developer feature. |
Maybe James was thinking about a build time warning? |
Yes, thank you. That's what I meant. Regarding the idea of a runtime option: I see the benefit but I also see significant risk. The fact that we have to expose |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of non-blocking nits. Happy to see this happening!
I think it's possible to add a test for it (at least by testing that adding a new module works), to feature-detect I think it makes sense to just use process.features
. But that can be added later since this is just a developer feature after all.
I'm not sure how to test this cc @nodejs/build |
If you want to do a one-off build you can run https://ci.nodejs.org/job/node-test-commit-custom-suites-freestyle/ with custom args passed to |
@richardlau i mean continuously. I guess we'd need to add a new box/configuration? |
52a14a6
to
c44133d
Compare
You probably wouldn't need a new box but probably would need a new job since you'd have to build Node.js with different options passed to configure. The nearest to that we currently have are the various sharedlibs jobs but we do not test every option that can be passed to The Build WG is currently very stretched though so there may not be much enthusiasm for adding another build configuration for a developer feature. |
re. new build configurations nodejs/build#1982 / #30057 would be ideal for this sort of thing but unfortunately it's stuck (nodejs/build#1982 (comment)). |
6ecfc90
to
dcc29cb
Compare
cctest failures when this is enabled:
|
I don't mind putting in a bit of work to extend the sharedlibs set to include new build configurations if the build configuration is above some poorly defined level of significance. Basically, is this anticipated to be a commonly used build option? Do we think this is going to grow into a more significant thing? If it's a key feature then I could do some work with some pointers on how to compile and run the tests with this option unless this is just some obscure thing in the corner that serves the use of a couple of people with special needs. |
I'd imagine people working on node core would just run with this option enabled (unless they were working specifically on the internal module loader itself) I don't think it's a huge priority to get testing infra for it though. |
(missed the button, sorry) |
Can this be tested on GH Actions instead? It would give some coverage and should be lower maintenance than doing it on Jenkins right now. |
dcc29cb
to
fb0f519
Compare
this is not ready to land, see #31321 (comment) |
@nodejs/collaborators re #31321 (comment) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
PR-URL: nodejs#31321 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
fb0f519
to
43fb6ff
Compare
landed in 43fb6ff |
PR-URL: #31321 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@devsnek should this go into |
PR-URL: nodejs#31321 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: #31321 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes