Preferred explicit module registration over __attribute__((constructor)) macro, since the latter doesn't work well with static library config. Also having an explicit registration is better in combination with BUILDFLAG for some internal modules https://github.com/electron/electron/blob/master/atom/common/node_bindings.cc#L189-L197.

although it might still be nice if we could ultimately end up using public Node.js APIs for this

The public api ends up defining the module registrators with __attribute__((constructor)) which doesn't work with the static library that is built in electron https://github.com/electron/electron/blob/master/BUILD.gn#L321. If referencing the constructors in the executable is the solution, I would prefer the manual registration over it. Similar to nodejs/node#16565

@deepak1556 I think it might just be an issue of copying/inlinine NODE_MODULE_CONTEXT_AWARE_CPP() into the Electron source rather than relying on the macro from node_binding.h, and maybe switching NM_F_LINKED to node::ModuleFlags::kLinked once nodejs/node#26457 makes its way into your Node.js fork?

The layout of the node_module struct itself is guaranteed to remain stable during a major Node.js release, so I think that should be okay.

Ah okay I misunderstood the public api switch then!

I think it might just be an issue of copying/inlinine NODE_MODULE_CONTEXT_AWARE_CPP() into the Electron source rather than relying on the macro from node_binding.h, and maybe switching NM_F_LINKED to node::ModuleFlags::kLinked
The layout of the node_module struct itself is guaranteed to remain stable during a major Node.js release

Yeah this should be good thing to do, given the guarantee. Thanks!

The linux tests failed here with

[401:0307/234309.568071:FATAL:zygote_host_impl_linux.cc(116)] No usable sandbox! Update your kernel or see https://chromium.googlesource.com/chromium/src/+/master/docs/linux_suid_sandbox_development.md for more information on developing with the SUID sandbox. If you want to live dangerously and need an immediate workaround, you can try using --no-sandbox.

Which is confusing 🤔

cc @nornagon

afaict that's a flake. I don't know what's causing it, perhaps some change in circleci's internal architecture? I've tried rerunning the test, let's see how it goes.

@nornagon As part of the setuid PR will the setuid sandbox become usable by the tests? Would be good to see that flake disappear as a result of that work if it is actually a flake.

i'm a little worried about why it only happens sometimes, it shouldn't be flaky.

@nornagon If we're happy the flake isn't somehow caused by this PR I think we're good to merge it now

i'm pretty sure i've seen it in other PRs and i can't imagine this would cause such a flake so i'm pretty sure we're good to go.

No Release Notes

/trop run backport-to 5-0-x

The backport process for this PR has been manually initiated,
sending your 1's and 0's to "5-0-x" here we go! :D

I was unable to backport this PR to "5-0-x" cleanly;
you will need to perform this backport manually.