Review requested:

• @nodejs/gyp
• @nodejs/startup
• @nodejs/tsc

@igorklopov, @jesec, @styfle could you look at what is documented/the proof of concept to confirm this is what we talked about in the next-10 mini-summit and that it's what is needed. If so would be great if you could help fill in the windows/osx versions.

@addaleax I had meant to mark this as a work in progress as I agree it's not complete (no tests, known memory leaks) but wanted some feedback/comments before going to far.

The commit messages should probably be src: experimental support for SEA (we don’t add usually add doc: when it’s documentation for source code changes and start with a lowercase letter).

sure

Even if experimental, this should come with tests.

agreed, it may be quite difficult without using an addon tool like LIEF which I don't think would make sense as part of the regular test suite. It would be easier if the tests were run nightly with additional dependencies. (EDITED), maybe the embedder API testing had a similar challenge, I'll look at that to see for ideas.

Are you sure that you want to start out with hex flags + wordexp as a format here? It seems like a pretty bad one for embedding data into an executable, esp. considering how non-trivial it is to properly escape for wordexp and how POSIX-specific it is. For example, the standard main argv format (strings concatenated with \0 as a separator) seems like a better (and easier) choice to start with.

I like the idea of using strings with \0 instead of the command line itself. That will make things easier for escaping.

In terms of the hex flags I was thinking it might help avoid endianess issues and was not too worried about the additional byes it would add

I think there should be some more explicit documentation on what the state we are ultimately working towards here is, because that does affect how the initial implementation would be evaluated. Is “compiling a bunch of command lines flags that replace the actually-passed ones” what we’re reaching for here? (Don’t get me wrong, that’s already very powerful!).

In the mini-summit from the discussions we had, we did not come up with anything that required more than bundling command line arguments and then running them given that '-e' could be used to specify the javascript that could be run. It might be more efficient if we also provided a binary section for data, but I don't think that is essential and could be added later on. If we had a list requirements that it would not address we could extend the format to cover those, but I'd suggest we keep it minimal until we have concrete requirements.

Even if experimental, this should come with tests.

agreed, it may be quite difficult without using an addon tool like LIEF which I don't think would make sense as part of the regular test suite. It would be easier if the tests were run nightly with additional dependencies. (EDITED), maybe the embedder API testing had a similar challenge, I'll look at that to see for ideas.

If it’s not included in the regular CI runs, then it’s something that at least needs to be optionally addable, like CITGM/stress test/etc..

Also, I don’t know about the exact challenges with LIEF, you are probably better to judge that, but I don’t see a strong reason for not including it in our regular test suite, conceptually.

In terms of the hex flags I was thinking it might help avoid endianess issues and was not too worried about the additional byes it would add

I wouldn’t worry too much about this, I think since we’re modifying a platform-specific binary file it’s okay to have platform-specific differences in that.

It might be more efficient if we also provided a binary section for data, but I don't think that is essential and could be added later on.

Yeah, I also don’t think that that’s essential – but it might be good to think about the path forward would potentially involve adding more binary sections, instead of using our own format for a single additional binary section (which may or may not be easier to work with, considering platform-specific differences).

If we had a list requirements that it would not address we could extend the format to cover those, but I'd suggest we keep it minimal until we have concrete requirements.

I think that’s fair, and I’m not expecting to start out with a list of concrete requirements, but again, it’s good to think about these things, because even a “minimal” solution still makes choices about what can be added on top of it.

For what it’s worth, I think the biggest reason why we would not currently look into using this approach here for our use case (and keep using boxednode) is that this approach does not enable bundling native addons. But I absolutely understand that that would be a fairly hard problem to solve when trying to avoid compilation.

For what it’s worth, I think the biggest reason why we would not currently look into using this approach here for our use case (and keep using boxednode) is that this approach does not enable bundling native addons. But I absolutely understand that that would be a fairly hard problem to solve when trying to avoid compilation.

There is some discussion in - https://sourceware.org/bugzilla/show_bug.cgi?id=11767 about loading shared libraries from memory but from related discussion it did not sound like it was going to move forward. Part of that discussion is writing out the shared libary to a file and then loading it which would be possible but also pretty ugly. Does boxnode effectively statically link in the addons or something else?

Part of that discussion is writing out the shared libary to a file and then loading it which would be possible but also pretty ugly

Yeah, it’s also possible to load them in-memory but that’s a huge amount of work that probably can’t be justified by this use case.

Does boxnode effectively statically link in the addons

Yes, that.

https://drive.google.com/drive/u/0/search?q=followup:actionitems

I think that link might just be working for you 🙂

https://drive.google.com/drive/u/0/search?q=followup:actionitems

sorry pasted wrong link, updated above and this is the right link:

https://sourceware.org/bugzilla/show_bug.cgi?id=11767