src: allow loading lib/ files from disk #9652

addaleax · 2016-11-17T01:59:55Z

Checklist

make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
tests and/or benchmarks are included
[½] documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

src

Description of change

Introduce a --internal-modules-source-dir= CLI option that allows
developers to test a modified version of the Node JavaScript source
files without rebuilding the Node executable and instead loading
the files from a directory on the disk.

This can be useful in situations where compiling and linking takes a
disproportionately long time, e.g. debugging Node core on slow machines
or inside of VMs, or installing a full compiler toolchain to build
Node does not seem reasonable.

This change does not document the CLI option in the official
documentation as it targets developers, not users, of Node.
Instead, a note is left in CONTRIBUTING.md.

CI: https://ci.nodejs.org/job/node-test-commit/6037/

mscdex · 2016-11-17T02:48:52Z

-1 it's already "bad enough" that we have an --expose-internals flag.

Fishrock123 · 2016-11-17T04:25:23Z

Interesting. So theoretically you could overload everything by changing the /dir/internal/bootstrap_node.js file? I think then you wouldn't even need to export the same things, would you?

addaleax · 2016-11-17T11:54:54Z

So theoretically you could overload everything by changing the /dir/internal/bootstrap_node.js file? I think then you wouldn't even need to export the same things, would you?

Yeah, pretty much. The only thing this doesn’t enable is adding new internal modules.

(Also, regarding: semver-minor… I don’t care much about the labelling but if possible, I’d try not to count this as an officially supported API.)

Fwiw, this would also reduce the required amount of patches to generate coverage.

evanlucas · 2016-11-17T11:59:47Z

I could have used this more than once before. I like the idea here.

Fishrock123 · 2016-11-17T15:38:44Z

I wonder, would it possible to only have these enabled in "dev" builds?

This does open an interesting set of possibilities, I don't think it exposes internals any more than we already do but people are very much at their own risk because doing anything will require interacting with process.binding().

evanlucas · 2016-11-17T15:39:53Z

@Fishrock123 maybe by adding an opt-in configure flag?

Fishrock123 · 2016-11-17T15:40:59Z

I'd figure running ./configure && make would build as "dev" by default and that release builds would be different.

evanlucas · 2016-11-17T15:42:14Z

@Fishrock123 what about all of the non official releases though, thinking like rpms and debs? I would think they run those themselves?

addaleax · 2016-11-17T19:38:39Z

You could make this dependent on the NODE_VERSION_IS_RELEASE bit or a configure flag, yeah. I mean, I would prefer to have this available from release builds so that you really don’t need to compile anything, but if there is no other way…

Like, adding this option doesn’t really open up new possibilities to anyone, it just makes doing this easier.

bnoordhuis

LGTM with two comments.

bnoordhuis · 2016-11-17T19:42:17Z

src/node_file.cc

+}
+
+MaybeLocal<String> InternalModuleReadFile(Environment* env, const char* path) {
+  uv_loop_t* loop = env->event_loop();


If it only uses env->event_loop(), it's IMO better to pass the event loop directly instead of taking on a dependency on Environment.

@bnoordhuis It also uses env->isolate() for creating the returned String… I could split that up into separate arguments but I’m not sure it’s worth it

bnoordhuis · 2016-11-17T19:43:49Z

src/node_javascript.cc

@@ -23,7 +25,24 @@ using v8::String;
 NODE_NATIVES_MAP(V)
 #undef V

+static MaybeLocal<String> MaybeLoadSourceFromDisk(Environment* env,
+                                                  const uint8_t* name) {
+  if (!internal_modules_source_dir) {


I have a mild preference for writing this out as if (internal_modules_source_dir == nullptr) { to make it clear that it's a pointer and not a boolean. Not a deal breaker, though.

@bnoordhuis done

bnoordhuis · 2016-11-17T19:47:59Z

You could make this dependent on the NODE_VERSION_IS_RELEASE bit or a configure flag

I don't think we need to go to such lengths to hide it. Just put a big "WARRANTY VOID IF USED" warning in the documentation and ruthlessly close any and all bug reports that people report.

jasnell · 2016-11-18T16:50:24Z

Oooo... I like it but definitely see the risks. That said, it would be extremely useful. A nice prominent runtime warning printed to the console would likely be a good thing also.

addaleax · 2016-11-18T19:54:51Z

I have thought of adding a warning and I’d be okay with that.

But: What exactly does that prevent that we don’t want people to do? If it’s about a graceful-fs-like situation, I am afraid it wouldn’t really make a difference, because anything that would use this feature programatically would also be able to automatically remove the warning.

Also, the CI failures are related but there doesn’t seem to be any kind of helpful output associated. I’ll try to look into that later.

(edit: probably fixed, new CI: https://ci.nodejs.org/job/node-test-commit/6068/)

addaleax · 2016-11-26T08:11:13Z

So… those who expressed that they are opposed to doing this, or those who think this should always come with a runtime warning: Could you describe a scenario in which this enables something that we don’t want to allow? I can’t think of anything that would not already be possible.

Fishrock123 · 2016-12-04T17:47:40Z

I think we should do this, I'm trying to run the coverage locally and it is super painful.

@addaleax One thing, maybe we should do this like _third_party_main? That would be a little bit more flexible when you only specify the entry file and everything else is relative as usual. Then you don't need to worry about always having a $DIR/lib/internal/bootstrap_node.js.

addaleax · 2016-12-04T21:44:26Z

One thing, maybe we should do this like _third_party_main?

Hm, what do you mean? Drop an extra file into lib/ and check for its existence?

Fishrock123 · 2016-12-04T21:48:13Z

@addaleax I mean have the flag be similar and specify only a specific entry file.

addaleax · 2016-12-04T22:11:39Z

@Fishrock123 So, a flag pointing to a file that is run instead of bootstrap_node? That would probably work, too, yeah, but it seems to target a pretty different kind of use case (the one people don’t like, namely only interfacing with the native bindings)?

Fishrock123 · 2016-12-04T22:53:55Z

@addaleax I don't think so because it would operate the same way only without locking you into the lib/internal/bootstrap_node.js path within the folder you are pointing to?

Fishrock123 · 2016-12-04T22:55:05Z

(As a note, I don't see any real risks in doing this either.)

addaleax · 2016-12-04T22:55:24Z

Oh wait, you are asking for a second flag that specifies the entry point path? Sure, that makes sense.

Introduce a `--internal-modules-source-dir=` CLI option that allows developers to test a modified version of the Node JavaScript source files without rebuilding the Node executable and instead loading the files from a directory on the disk. This can be useful in situations where compiling and linking takes a disproportionately long time, e.g. debugging Node core on slow machines or inside of VMs, or installing a full compiler toolchain to build Node does not seem reasonable. This change does not document the CLI option in the official documentation as it targets developers, not users, of Node. Instead, a note is left in `CONTRIBUTING.md`.

addaleax · 2016-12-05T00:34:49Z

@Fishrock123 Could you take a look at 991e955 and see if that’s what you had in mind?

@mscdex You gave this a clear “-1”… could you describe the kind of undesirable scenario you think we might enable with this?

Fishrock123 · 2016-12-05T00:40:29Z

@addaleax I was hoping to just have one flag to an entry point file but perhaps that isn't feasible, in that case specifying just the directory could be fine for now?

addaleax · 2016-12-05T00:48:08Z

@Fishrock123 We could do that too, yes. It would just make the original use case that I wanted to support (namely easier hacking on lib/) a bit trickier?

mscdex · 2016-12-05T00:48:10Z

@addaleax I'm just concerned about module authors relying on this and potential support issues that could arise when it is used as a "regular feature" and not for temporary/debugging purposes. If this is absolutely necessary, what about having a compile-time flag that enables the runtime flag?

addaleax · 2016-12-05T00:53:37Z

@mscdex Yeah, I get that, but I don’t think this actually enables module authors to do anything they can’t already do?

What about making it a compile-time flag instead that enables the behavior?

We could do that but I still feel like that largely misses the point… what I’d like to see is to have the option to debug Node without compiling it, e.g. because I’m in a VM for which I can’t allocate the proper resources (that’s not a purely theoretical concern for me). Putting it behind a compile-time flag would erase that benefit completely…

mscdex · 2016-12-05T00:59:28Z

@addaleax It doesn't really matter anyway, since it seems I'm in the minority here.

addaleax · 2016-12-05T01:02:22Z

@mscdex That doesn’t mean it’s not worth discussing the dangers here… I certainly don’t want to be responsible for another graceful-fs kind of situation

jasnell · 2016-12-05T01:12:57Z

A compile time flag to enable it would be worthwhile I think. I understand the limitation that places on things but @mscdex's concerns make sense here

…

On Sun, Dec 4, 2016 at 5:02 PM Anna Henningsen ***@***.***> wrote: @mscdex <https://github.com/mscdex> That doesn’t mean it’s not worth discussing the dangers here… I certainly don’t want to be responsible for another graceful-fs kind of situation — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#9652 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAa2eXPY0tAr-0msHsdvx0sO17O-fufdks5rE2KigaJpZM4K0vOB> .

Fishrock123 · 2016-12-05T02:23:28Z

Ok so, technically overloading the entry file before process gets a chance to set up exposes all sorts of process setup hackery. I assume that is why _third_party_main loads when it does.

A compile-time flag ought to rid us of any responsibility I think.

@addaleax Does that hamper coverage? Do we build on the coverage machine or not?

addaleax · 2016-12-05T02:30:05Z

A compile-time flag ought to rid us of any responsibility I think.

Right, and if people are really convinced that that is the best way forward, that’s okay. But, again, this PR doesn’t open new possibilities to anyone, it just makes certain things easier; so I’m not sure what putting it behind a compile-time flag would gain.

If there is some scenario in which this feature can be misused for something we don’t want to support, I’d be totally on board with putting it behind a flag.

Do we build on the coverage machine or not?

I am pretty sure we do. So whether this is behind a compile-time flag or not doesn’t matter for the coverage use case.

Fishrock123 · 2016-12-05T03:58:28Z

Gaining access to process before setup does open new possibilities though. Once we use the setup helpers on it they are removed from the object so they are only available the first time.

Whether any of those helpers are actually significantly interesting to worry about is a different story. I'd wonder if pretty much anyone outside of core contributors knew of this particular setup behavior.

addaleax · 2016-12-05T18:27:23Z

Gaining access to process before setup does open new possibilities though. Once we use the setup helpers on it they are removed from the object so they are only available the first time.

Whoever uses the flag(s) exposed by this PR programatically would have to do so using child processes, and you can do pretty much literally anything with them. Fwiw, here’s a proof of concept that should give you a patched node executable which loads its entry point from an arbitrary file.

Fishrock123 · 2016-12-05T19:08:27Z

"good" old process.binding('natives'). Sigh

Fishrock123 · 2016-12-05T19:10:01Z

Ok well we might as well land this in some form then.

@addaleax Do you think it would be possible to combine the two flags into just one flag pointing to an entry file, or are both needed?

addaleax · 2016-12-05T19:12:35Z

@Fishrock123 Yes, that would work. The thing is just that once you have the code for either one, it’s pretty easy to add the other one, and I think the one specifying a directory to load from is more useful as long as the use case is hacking on node core?

Fishrock123 · 2016-12-05T19:14:38Z

Sure, seems fine. I just thought it may be worthwhile to have it work similar to _third_party_main. :)

Fishrock123

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

Fishrock123 · 2016-12-14T15:42:37Z

@mscdex does the above info address your concerns? I think the benefits here should outweigh the negatives?

mscdex · 2016-12-14T17:32:50Z

@Fishrock123 Not really.

jasnell

I'm generally not comfortable at all with this being enabled in release builds. It makes sense allowing it as a configure option for dev builds but I want the ability to turn it off for releases. I would also still prefer to see a prominent warning printed to stderr when this is used.

addaleax · 2017-01-12T18:04:38Z

Okay, given the feedback I’m getting here I’m closing this. Imho, having this behind a compile-time flag would turn this feature into a nearly pointless one; if anybody thinks differently and wants to pick this up where I left it, feel invited to do so.

jkrems · 2017-11-26T17:30:39Z

Random data point: I'm working on some lib/ changes for dynamic import right now and remembered this PR fondly (even the mostly noop rebuild costs a few seconds each time). Not sure how frequent these JS-heavy changes are though.

targos · 2017-11-26T17:55:53Z

I would also like to have this. It would already be extremely helpful behind a compile flag

devsnek · 2017-11-26T17:59:40Z

i find myself often doing ./node --expose-internals and then require('./lib/internal/...') on the file that i changed, and of course if i change more than one i have to recompile

addaleax · 2017-11-26T18:01:40Z

@jkrems @targos @devsnek The code has changed quite a bit so you might want to do a fresh start if you want to pick this up again.

I still think putting it behind a compile flag is both pointless for the use cases I was thinking of (i.e. debugging problems in released versions of Node) and unwarranted in that this feature is extremely hard to use for anything but Node core development, but that shouldn’t stop any of you from opening a PR with whatever version you have in mind. :)

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. doc Issues and PRs related to the documentations. labels Nov 17, 2016

addaleax added lib / src Issues and PRs related to general changes in the lib or src directory. and removed doc Issues and PRs related to the documentations. labels Nov 17, 2016

mscdex added the module Issues and PRs related to the module subsystem. label Nov 17, 2016

Fishrock123 added the semver-minor PRs that contain new features and should be released in the next minor version. label Nov 17, 2016

bnoordhuis approved these changes Nov 17, 2016

View reviewed changes

addaleax force-pushed the internal-modules-from-disk branch from cef84c1 to 5385b5a Compare November 18, 2016 20:29

addaleax force-pushed the internal-modules-from-disk branch from 5385b5a to c2c5dbe Compare November 30, 2016 02:58

addaleax force-pushed the internal-modules-from-disk branch from c2c5dbe to 991e955 Compare December 5, 2016 00:33

Fishrock123 approved these changes Dec 5, 2016

View reviewed changes

jasnell requested changes Jan 11, 2017

View reviewed changes

addaleax closed this Jan 12, 2017

refack mentioned this pull request Nov 4, 2018

Discussion: How should the Node executable + Node shared library be distributed? #24028

Closed

addaleax deleted the internal-modules-from-disk branch October 12, 2019 17:05

addaleax mentioned this pull request Oct 12, 2019

Change lib files without compiling the entire project again nodejs/help#2228

Closed

src: allow loading lib/ files from disk #9652

src: allow loading lib/ files from disk #9652

Conversation

addaleax commented Nov 17, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

mscdex commented Nov 17, 2016

Fishrock123 commented Nov 17, 2016 • edited Loading

addaleax commented Nov 17, 2016

evanlucas commented Nov 17, 2016

Fishrock123 commented Nov 17, 2016

evanlucas commented Nov 17, 2016

Fishrock123 commented Nov 17, 2016

evanlucas commented Nov 17, 2016

addaleax commented Nov 17, 2016

bnoordhuis left a comment

Choose a reason for hiding this comment

bnoordhuis Nov 17, 2016

Choose a reason for hiding this comment

addaleax Nov 17, 2016

Choose a reason for hiding this comment

bnoordhuis Nov 17, 2016

Choose a reason for hiding this comment

addaleax Nov 17, 2016

Choose a reason for hiding this comment

bnoordhuis commented Nov 17, 2016

jasnell commented Nov 18, 2016

addaleax commented Nov 18, 2016 • edited Loading

addaleax commented Nov 26, 2016

Fishrock123 commented Dec 4, 2016

addaleax commented Dec 4, 2016

Fishrock123 commented Dec 4, 2016

addaleax commented Dec 4, 2016

Fishrock123 commented Dec 4, 2016 • edited Loading

Fishrock123 commented Dec 4, 2016

addaleax commented Dec 4, 2016

addaleax commented Dec 5, 2016

Fishrock123 commented Dec 5, 2016

addaleax commented Dec 5, 2016

mscdex commented Dec 5, 2016 • edited Loading

addaleax commented Dec 5, 2016

mscdex commented Dec 5, 2016

addaleax commented Dec 5, 2016

jasnell commented Dec 5, 2016 via email

Fishrock123 commented Dec 5, 2016 • edited Loading

addaleax commented Dec 5, 2016

Fishrock123 commented Dec 5, 2016 • edited Loading

addaleax commented Dec 5, 2016

Fishrock123 commented Dec 5, 2016

Fishrock123 commented Dec 5, 2016 • edited Loading

addaleax commented Dec 5, 2016

Fishrock123 commented Dec 5, 2016

Fishrock123 left a comment

Choose a reason for hiding this comment

Fishrock123 commented Dec 14, 2016

mscdex commented Dec 14, 2016

jasnell left a comment

Choose a reason for hiding this comment

addaleax commented Jan 12, 2017 • edited Loading

jkrems commented Nov 26, 2017

targos commented Nov 26, 2017

devsnek commented Nov 26, 2017 • edited Loading

addaleax commented Nov 26, 2017

addaleax commented Nov 17, 2016 •

edited

Loading

Fishrock123 commented Nov 17, 2016 •

edited

Loading

addaleax commented Nov 18, 2016 •

edited

Loading

Fishrock123 commented Dec 4, 2016 •

edited

Loading

mscdex commented Dec 5, 2016 •

edited

Loading

Fishrock123 commented Dec 5, 2016 •

edited

Loading

Fishrock123 commented Dec 5, 2016 •

edited

Loading

Fishrock123 commented Dec 5, 2016 •

edited

Loading

addaleax commented Jan 12, 2017 •

edited

Loading

devsnek commented Nov 26, 2017 •

edited

Loading