-
Notifications
You must be signed in to change notification settings - Fork 15.3k
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
Provide binary releases without bundled MSVC++ and UCRT Runtimes #14617
Comments
👋 Thanks for opening your first issue here! If you're reporting a 🐞 bug, please make sure you include steps to reproduce it. We get a lot of issues on this repo, so please be patient and we will get back to you as soon as we can. To help make it easier for us to investigate your issue, please follow the contributing guidelines. |
Electron 4.0.x will be linked with a statically-linked CRT (i.e. built with |
That's already been done and it did end bad. It ended up causing issues every single time you used a standard container (heap corruption, intermittent crashes, etc.) I don't recall the exact cause but there was an issue on it here: #9443 Is there a reason this is being considered again? In addition, our application can't ship the runtime, even in statically linked form due to GPL restrictions. |
Statically linking the CRT would not end well. It would introduce new crashes and also bring back old crashes that were fixed by #9443. If electron chooses to statically link again, we'd either have to use mingw32 to compile the client-side module (risking ABI incompatibilities) or never upgrade to 4.0 until the decision is reverted. I have several arguments against statically linking the CRT though: App/Binary Size will grow drastically, critical fixes for the CRT would require an electron update (which will take a bit, then go through testing, all the time leaving users vulnerable) and introduces new crashes (as well as reintroduces old ones). |
The statically linked runtime is possible because electron 4.0 will be a single statically-linked binary, without Can you expand on the GPL restrictions? The one exception to the statically-linked binary is ffmpeg, which remains a separate dll. Overall download size in Electron 4 is actually a little smaller than 3 currently. |
All this said, it's not crazy that we could link the dynamic CRT instead of the static CRT if we decide that's the right way to go. The decision was made this way because that's how Chromium works, and in general, the closer we hew to how Chromium works, the easier it will get to stay up to date with Chromium. |
Disclaimer: IANAL. We use both GPLv3 and GPLv2 licensed libraries. GPLv3 which has a broader idea of what a "System Library" is. It is mentioned in their quick guide that shipping a proprietary C runtime is okay in that exception under the concept of a "Major Component" (though nothing is mentioned for static libraries still as far as I can tell so not 100% sure on that one even if this goes through). But we have GPLv2 libraries that we distribute that don't have the same type of system library exception. If we use a node module that uses that GPLv2 library, suddenly we may not be compliant because of the runtime shipped inside of or next to electron (as far as I can tell anyways). GPL can get kinda confusing here frankly, especially when you consider this is an interpretation rather than a rule. All that said, I'm more concerned about the technical limitations than anything else. If there's not significant overhead to statically linking, I'm more inclined to be open to it. What I really don't want is to suddenly have a change that breaks almost every native node module ever, which was a very frustrating period of time. |
Yeah, we definitely don't want to break every native module! Electron's own tests include a simple native module test using the |
If I'm honest, I'm not sure I'm going to have time to do this any time soon. I'm also not aware of a pattern of behavior that caused the conflict. Our reproduction case was to create a std::stringstream or use particular functions/containers and we would get a stack trace (sometimes an unusable one) that would lead into the destructor of that container or it would corrupt our heap causing rather mysterious crashes at some seemingly random time later when we interacted with the heap. I'm not sure how you could reliably make a test for this behavior. |
Will it also be smaller than an electron without any MSVC/CRT runtime bundled? We'd like to keep app size down and compatibility up, and static MSVC/CRT is not helping the former one, while the latter one is to be determined (everything broke the last time CRT was statically compiled in, so I don't think it'll be different this time). I can get a basic electron-crt-tests module going that will have four binaries (vs2013, vs2015, vs2017 pre-abi-change and vs2017 post-abi-change binaries), which should help figuring out if the static crt linking causes issues. |
Things are a lot different now than they were :)
I doubt it, since Electron still depends on the CRT, it's just linked statically now instead of dynamically. Have you been building a custom release with those DLLs removed? In terms of app size, the CRT accounts for less than 1% of the size of electron overall, so I don't think app size is a good argument for supporting a different CRT linkage scheme. An electron-crt-tests module sounds great! Looking forward to seeing it :) |
They really aren't. CRT and MSVCR are still the same way as they were last year when we tried to do the dynamic-static-mixed linking stuff, and newer CRT versions (2017) actually have an ABI breaking change, so statically linking breaks even more (for reference: std::align() now behaves different from before, plus some other stuff). This also makes us depend on electron updates for security fixes to the CRT/MSVCR, things might end up broken and will take until the next electron update (whenever that may be) to fix. That is not something I'm looking forward to.
I have made local builds with electron not bundling the CRT/MSVCR and it reduced the .zip size from 52mb to 47.2mb. That's a 9.2% size reduction just from removing the bundled CRT/MSVCR and using the system installed one. I do not know how you got the 1% number, since I have no build option that gets me close to that - if you could elaborate how you got that number, I can repro it locally and see if it would be okay in terms of size. |
I might have bumped into this today when attempting to build a native module for electron 4 with cmake-js, which builds with I'm surprised that Tomorrow I'll try a test build of Electron that links against the CRT dynamically and see if I run into the same issue with modules built with |
@nornagon We are also running into issues with Electron 4. Our node modules are dynamically linking the CRT. We get crashes when using |
it can be actually fixed by linking the CRT before
|
Ah interesting... exporting those STL symbols from electron.lib seems wrong to me. |
definitely, if we can figure out how to fix |
In which part of the build system is this static linking taking place? |
Looks like there is a |
Closing this since it's been a while and we don't have plans to change this. In general, you should not expect C++ calls between Electron and native modules to work. Electron now uses libc++ instead of MSVC++'s runtime, which is even more incompatible. N-API is the way to go. |
Is your feature request related to a problem? Please describe.
In 2.x and 3.x releases, electron currently ships with a bundled MSVC++ and UCRT Runtime, which cause native C++ modules that were built with Microsoft Visual Studio 2015 or 2017 to behave differently than expected. Even if the user installs the latest version on their system, the local copy next to the electron.exe is loaded first, resulting in unexpected behavior.
For our case (Streamlabs OBS with native module OBS Studio for Node.JS) this results in C++11 (and newer) features being locked away, as they can cause instability. We have observed std::stringstream crashing the application with a heap corruption due to this rather old runtime being shipped with electron.
Describe the solution you'd like
A binary release without bundled MSVC and UCRT Runtimes as an alternative to those with them bundled. This will allow us to properly use C++11 (and newer) features in native modules without being restricted by a very old, possibly buggy and unsafe, MSVC and UCRT library.
Describe alternatives you've considered
We've attempted to use 3.0.0-beta which ships with 2017 binaries, however early tests however did not work out due to a bug that prevented our code from working in 3.0.0-beta. In the short term, I'll try to modify electron-builder to forcefully remove those files as they at times make debugging impossible (electron 2.0.8 has a vcruntime140.dll that doesn't exist in the Microsoft Symbol Server or Electron Symbol Server - Visual Studio 2017 can't find it in either).
Steps to reproduce the C++11 issue on electron 2.0.x
The text was updated successfully, but these errors were encountered: