chore: fix ambiguous reference gcc compile error

[brjsp]

Description of Change

When compiling electron 20.1.4 on linux with gcc, the compiler cannot determine what Observer refers to. I needed to apply this patch to get it to compile.

Release Notes

Notes: none

[MarshallOfSound]

You shouldn't be building Electron with gcc. Our only supported build system is Chromium's clang + libcc. Anything else will almost certainly either not compile or crash/fail at runtime in mysterious ways.

[brjsp]

Just to be clear: I am not building Electron to distribute with my application. I am openSUSE's package maintainer of Electron, we build a single system copy to be used by several apps we distribute in the repo (Element, Signal, in the future also Bitwarden and possibly VSCode).

We cannot build stuff with LLVM's libc++ for technical reasons — system libraries (notably abseil) link to GNU libstdc++ and use its ABI. We would really like to build Electron with clang, but it fails to build libstdc++'s std::optional, making it unusable for our use case.

It's worth mentioning that while Chromium does not officially support GCC, they do accept patches fixing compile errors, as they are likely indicative of incorrect use of the C++ language.

[MarshallOfSound]

we build a single system copy to be used by several apps we distribute in the repo (Element, Signal, in the future also Bitwarden and possibly VSCode).

This does not work and is not a supported or intended use of Electron. Many apps, just in that list for instance VS Code, use a patched / different / internal build of Electron to the one upstream provides. If you build your own you are likely shipping a broken or less secure version of that application.

We would really like to build Electron with clang, but it fails to build libstdc++'s std::optional, making it unusable for our use case.

Both Electron and Chromium build with Clang and libcxx, not sure how you'd be running into an issue that neither of our build systems run into. Or is this a side effect of using clang and not using chromiums libcxx (which is itself a modified version of llvm's libcxx)

It's worth mentioning that while Chromium does not officially support GCC, they do accept patches fixing compile errors, as they are likely indicative of incorrect use of the C++ language.

Familiar with Chromiums patch policy, it's not like this patch is huge so we'll probably take it too 🤷 but I really want to emphasize the points above. What you're trying to do and your desired use case for it are a recipe for disaster.

[brjsp]

VSCode hooks into Electron in unsupported ways that we had to patch around, but they use a stock Electron binary. They were apparently building they custom version back in the libchromiumcontent days, but i wasn't around back then. [Maybe the Arch packagers might be more familiar with that]

We don't have the resources to ship several different versions of Electron — we've got ~five people doing this in their spare time. We pick a single version all these four apps work with. And anyway, vendoring your own copy of a runtime — especially one as large as Electron! — is something that has never been allowed by major free software distros. Debian and Fedora have the same policy.

Or is this a side effect of using clang and not using chromiums libcxx (which is itself a modified version of llvm's libcxx)

The current problem I'm having is that Chromium's bundled abseil uses a custom implementation of absl::optional, while our abseil follows the upstream default of typedefing it to std::optional.

I noticed Clang 15 just got released — I should check if they fixed that problem soon.

[ckerr]

Setting compilers aside and just looking at this patch at the language level, I slightly prefer the PR version because it's more explicit. Approving on those grounds alone.

Everything that @MarshallOfSound said above about is right, though: shipping a binary that was built with an unsupported build config is risky behavior.

[ckerr]

Please push a formatting commit to the PR to make the linter happy

https://app.circleci.com/pipelines/github/electron/electron/58004/workflows/4de07e0d-033a-40eb-aa6e-f60afbb35b22/jobs/1317338

-void ElectronSerialDelegate::AddObserver(content::RenderFrameHost* frame,
-                                         content::SerialDelegate::Observer* observer) {
+void ElectronSerialDelegate::AddObserver(
+    content::RenderFrameHost* frame,
+    content::SerialDelegate::Observer* observer) {
   observer_list_.AddObserver(observer);
   auto* chooser_context = GetChooserContext(frame);
   if (!port_observation_.IsObserving())
     port_observation_.Observe(chooser_context);
 }
 
-void ElectronSerialDelegate::RemoveObserver(content::RenderFrameHost* frame,
-                                            content::SerialDelegate::Observer* observer) {
+void ElectronSerialDelegate::RemoveObserver(
+    content::RenderFrameHost* frame,
+    content::SerialDelegate::Observer* observer) {
   observer_list_.RemoveObserver(observer);
 }

[Prinzhorn]

but I really want to emphasize the points above. What you're trying to do and your desired use case for it are a recipe for disaster.

I've just randomly stumbled in here yesterday and wanted to point something out (and strongly agree with the above).

We pick a single version all these four apps work with

Maybe I'm missing the point, feel free to correct me if the following doesn't make sense:

What does "work" mean in this context? It launches? Every Electron patch release can patch bugs or introduce new ones. VS Code, Slack etc. might launch with a specific version, say "20.1.3". But that doesn't mean certain features won't break, because they require a fix in "20.1.4". And they might never have shipped a version with "20.1.3", because the bug was introduced in "20.1.2" and they waited for "20.1.4". It's impossible to guarantee that you don't completely break an app by shipping a different Electron version that they ran in CI, even if they use a vanilla unaltered Electron binary. Somewhat related: that's why I personally am very skeptical when it comes to something like Tauri: running on different webviews and versions that you cannot control sounds like testing hell.

[brjsp]

@Prinzhorn Slack is not open source, we cannot patch or distribute it, and thus is not relevant to our discussion.

I broadly agree with Gentoo's take on pinning dependencies and ask you to read it.

As for why your model means dependency hell for us, let me provide an example: for several months the official Electron binaries were crashing on startup on every modern linux system (libc6 ≥ 2.34). This is something that we could patch in our builds, but did nothing for third-party software shipping vendored electron, which was unusable.

My opinion, seen through distro-colored glasses, is: apps should never, ever, try to vendor something as complicated as a browser engine. That is the operating system's job.

I pledge you to consider #673 as an official option, and not only as a semi-supported developer hack.

I haven't seen Tauri in the wild, but have seen (on Windows) Edge WebView2 which looks like a step in the good direction.

[Prinzhorn]

@brjsp thanks for the links, these were interesting reads. I think we're both looking at this from two entirely different perspectives and it's interesting to see your perspective.

I broadly agree with Gentoo's take on pinning dependencies and ask you to read it.

How would you apply a term such as "stable public API" to something like Chromium? Everything it does, every single CSS property and JavaScript API is part of Chromiums "public API". It's moving so fast and breaks all the time that there is nothing stable about it. That's why we need to vendor or else a patch release will break the CSS property that I was using in a very specific way. What I mean is that the problem starts way earlier than Electron or even Chromium. Everything down the line comes with vendored dependencies. I don't think unvendoring Electron from some apps does much and will instead break apps, because the whole supply chain already suffers from Hyrum’s Law (linked in the Gentoo post).

I pledge you to consider #673 as an official option, and not only as a semi-supported developer hack.

I can't speak for the Electron team, but of course I would be +1 if it actually worked. But we're seeing Java desktop apps ship the entirety of the JRE to have a consistent experience for all users. And I don't see Electron fixing this. But this is probably not the right place to discuss this. But if there will be any further discussion in a different place I'd be happy to be invited.

[release-clerk]

No Release Notes

[trop]

I have automatically backported this PR to "20-x-y", please check out #35733

[trop]

I have automatically backported this PR to "21-x-y", please check out #35734