search

electron / libchromiumcontent

Shared library build of Chromium’s Content module
MIT License
485 stars 183 forks source link

chore: [gn] build icu & v8 statically in GN build #566

Closed nornagon closed 6 years ago

nornagon commented 6 years ago

When building Electron with GN in release mode, all code is linked statically into a single binary, including node & v8. This is in contrast to the GYP build, in which node, v8 and icu are linked as a shared library (due to openssl symbol conflicts).

This change adds a is_electron_gn_build flag which defaults to false, allowing those libraries to be built statically in the GN build, but be built as a shared library when libcc is to be consumed by the GYP build of Electron.

deepak1556 commented 6 years ago

Would it be possible to skip those patches in the gn build as opposed to introducing a new flag ? If its more work then I am fine with this approach.

nornagon commented 6 years ago

Ah, interesting thought! I'll give it a shot.

nornagon commented 6 years ago

I looked into selectively applying the build_gn patch only in the non-GN build, and I think it's simpler to do it the way that's already in this PR. Doing the selective application would mean:

  1. Adding a special annotation in .patches.yaml, e.g. skip_in_gn_build: true
  2. Reading that annotation in lib/patches.py
  3. Adding a filter in PatchesList#apply to filter out the skip_in_gn_build patches
  4. Adding an argument to the apply-patches script to select whether to filter out those patches
  5. Passing that argument in the gclient hook in electron/electron's DEPS file

It's possible, but it adds a nontrivial amount of complexity to the patching system, which is at present delightfully simple. Further, when we eventually switch to GN-build-only, all that complexity would become useless.

One possible reason to go ahead with the filtering option would be if we wanted to shift our MAS and mipsel patches over to such a system. But I don't see any particular reason to do so; the current system is working fine for those. The difference with this particular case is that it would be default-on, triggered-off, rather than the MAS/mipsel patches which are default-off, and only applied in certain conditions.