fix: export libuv symbols

[koonpeng]

Description of Change

Forces libuv symbols to be exported, this fixes #24428.

Checklist

• PR description included and stakeholders cc'd
• npm test passes
• tests are changed or added
• relevant documentation is changed or added
• PR title follows semantic commit guidelines
• PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.
• This is NOT A BREAKING CHANGE. Breaking changes may not be merged to master until 11-x-y is branched.

Release Notes

Notes: Fixed undefined symbol error when loading native modules that uses uv_dlopen

[welcome]

💖 Thanks for opening this pull request! 💖

We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix.

Examples of commit messages with semantic prefixes:

• fix: don't overwrite prevent_default if default wasn't prevented
• feat: add app.isPackaged() method
• docs: app.isDefaultProtocolClient is now available on Linux

Things that will help get your PR across the finish line:

• Follow the JavaScript, C++, and Python coding style.
• Run npm run lint locally to catch formatting errors earlier.
• Document any user-facing changes you've made following the documentation styleguide.
• Include tests when adding/changing behavior.
• Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

[MarshallOfSound]

I seem to recall discussing this with @codebytere and this isn't exactly what we want to be doing, the symbols are being stripped forcefully adding them again is just working around a problem that we are causing

[koonpeng]

Thanks for the quick respond! For context, I got an undefined symbol error when trying to load a native module that works in node, the reason I found is because node exports a few extra symbols that electron is missing.

I followed the docs and stripped the symbols made a patched release build here https://github.com/koonpeng/electron/releases/tag/v9.0.5. Comparing that to the official release,

teokp@teokp-Desktop:~/Downloads$ nm -D ./electron-v9.0.5-linux-x64/electron | awk '{print $3}' > default.txt 
teokp@teokp-Desktop:~/Downloads$ nm -D ./electron-v9.0.5-linux-x64-koonpeng/electron | awk '{print $3}' > patched.txt
teokp@teokp-Desktop:~/Downloads$ git diff default.txt patched.txt
diff --git a/default.txt b/patched.txt
index c188ec7..29e92b7 100644
--- a/default.txt
+++ b/patched.txt
@@ -1589,6 +1589,10 @@ uv_cpu_info
 uv_cwd
 uv_default_loop
 uv_disable_stdio_inheritance
+uv_dlclose
+uv_dlerror
+uv_dlopen
+uv_dlsym
 uv_err_name
 uv_err_name_r
 uv_exepath

It seems that only 4 more libuv symbols are added, filesize remains pretty much identical. I'm not very sure if this is the correct way to fix it so I'm happy to hear other suggestions.

[codebytere]

I understand the motivation behind this and also want to fix it, but I don't believe that passing a reference to a generate static library file in this manner is the best choice here - it's somewhat brittle and I'd prefer to look into determining why the small handful of missing symbols are missing and fix that more directly as Sam says above.

[koonpeng]

I explored abit further on this, it looks like node's build script also uses --whole-archive https://github.com/nodejs/node/blob/b0b52b2023f5cd0df4ae921850815586b4313dca/node.gypi#L176. This flag is used when 'force_load=="true"'. force_load is only set when building as an executable, I think electron's node is built as a static library so it is correct that it doesn't use the flag.

I'm not sure how to go about this, in node's docs it says that libuv is purposefully re-exported and should be usable by native modules. If electron aims to support native modules, should it add --whole-archive even though it's building node as a static library?

Node.js includes other statically linked libraries including OpenSSL. These other libraries are located in the deps/ directory in the Node.js source tree. Only the libuv, OpenSSL, V8 and zlib symbols are purposefully re-exported by Node.js and may be used to various extents by Addons. See Linking to libraries included with Node.js for additional information.

[zcbenz]

These symbols are missing in Electron because they are not used anywhere in our codebase and the linker removes them automatically. This problem happens on all platforms so a linux-only fix would not be enough.

We can probably explicitly reference the symbols via -u linker flag: https://stackoverflow.com/a/8907422.

[koonpeng]

node uses --whole-archive to export all the symbols in libuv, I think electron should follow it instead of explicitly adding certain symbols with -u?

Actually the problem doesn't happen in Windows because libuv is built as a shared library, I wasn't able to test on mac so I don't know if it works. I have a test project here https://github.com/koonpeng/electron-uv-test. I can try adding a test case that use that module and see if it works on the CI, in the meantime I will convert this to a draft.

[zcbenz]

Thanks for providing the test project, I confirm that this problem also happens on macOS.

I'm good with the --whole-archive solution if you can add the flag for both Linux and macOS. I think it is fair use here since the lack of symbols is predictable, and exporting all symbols of libuv is expected behavior for users.

[koonpeng]

I tried using force_load and whole-archive flags like in node but I'm still getting symbol errors in the mac tests. It's hard for me to test without a mac.

[zcbenz]

Thanks for adding the test, I have added a commit to make it work on mac, it turns out we should use the -force_load flag there.

[codebytere]

@koonpeng can you please rebase this on master?

[codebytere]

Failure is known on macOS - will merge when the Linux rerun passes.

[zcbenz]

The failing mac test is unrelated.

[welcome]

Congrats on merging your first pull request! 🎉🎉🎉

[release-clerk]

Release Notes Persisted

Fixed undefined symbol error when loading native modules that uses uv_dlopen