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

GN build #12837

Merged
merged 1 commit into from May 15, 2018

Conversation

Projects
None yet
6 participants
@nornagon
Contributor

nornagon commented May 4, 2018

This PR adds GN build infrastructure for building on macOS. This build system is still in the experimental phase while we continue to explore and look for the best way of integrating with Chromium's GN build system. This doesn't affect the GYP build, which should continue to function unimpacted by these changes. As of this PR, the build will only work as a component build on macOS.

Depends on electron/libchromiumcontent#537 and #12738.

The first commit in this PR updates #include paths to not refer to vendor/, since the GN build checks out those repositories in different locations.

Instructions for trying out the GN build on Mac (once this is merged):

$ mkdir electron-gn && cd electron-gn
$ cat > .gclient <<-GCLIENT
solutions = [
  {
    "url": "https://github.com/electron/electron",
    "managed": False,
    "name": "src/electron",
  },
]
GCLIENT
$ gclient sync --with_branch_heads --with_tags --nohooks && gclient runhooks
$ cd src
$ export CHROMIUM_BUILDTOOLS_PATH=`pwd`/buildtools
$ gn gen out/Default --args='cc_wrapper="sccache" is_component_build=true use_jumbo_build=true root_extra_deps=["//electron"]'
$ ninja -C out/Default electron:electron_app
$ ./out/Default/Electron.app/Contents/MacOS/Electron

Background information

This is a substantial structural change to how electron is checked out. Instead of git submodules, in the GN build electron and its dependencies (including chromium) are checked out with gclient. The structure of an electron checkout in the GN build world is:

.
├── .gclient
└── src
    ├── base
    ├── build
    ├── components
    ├── content
    ⋮
    ├── electron
    │   ├── atom
    │   ├── brightray
    │   ⋮
    ├── libchromiumcontent
    ⋮
    ├── third_party
    │   ├── WebKit
    │   ├── electron_node
    │   ├── native_mate
    │   ⋮
    ⋮

This is coordinated through DEPS files instead of git submodules.

@nornagon nornagon requested a review from electron/reviewers as a code owner May 4, 2018

@nornagon nornagon referenced this pull request May 4, 2018

Merged

GN build support #537

@zeke

This comment has been minimized.

Show comment
Hide comment
@zeke

zeke May 6, 2018

Member

Just curious: Is DEPS a convention? If so, from what project/language/community?

Member

zeke commented May 6, 2018

Just curious: Is DEPS a convention? If so, from what project/language/community?

@nornagon

This comment has been minimized.

Show comment
Hide comment
@nornagon

nornagon May 7, 2018

Contributor

@zeke: DEPS is used by gclient to discover submodules. See the depot_tools docs for more (though those docs aren't great sadly 😞)

Contributor

nornagon commented May 7, 2018

@zeke: DEPS is used by gclient to discover submodules. See the depot_tools docs for more (though those docs aren't great sadly 😞)

@nitsakh

nitsakh approved these changes May 9, 2018

Excellent! 👍

@deepak1556

Have verified this locally, built with the following configuration

cc_wrapper = "sccache"
is_component_build = true
use_jumbo_build = true
is_electron_build = true
is_debug = true
symbol_level = 2
use_allocator = "none"
enable_nacl = false
proprietary_codecs = true
is_component_ffmpeg = true
ffmpeg_branding = "Chrome"
v8_promise_internal_field_count = 1
v8_typed_array_max_size_in_heap = 0
root_extra_deps = [ "//electron" ]

I am good with merging this as experimental, to allow work on this incrementally. @nornagon what do you think about adding a doc entry with the setup described in the PR description. Also Would like to get approval from @zcbenz and @alexeykuzmin before merging.

Show outdated Hide outdated DEPS

@deepak1556 deepak1556 requested review from alexeykuzmin, zcbenz and alespergl May 10, 2018

"//ppapi/proxy",
"//ppapi/shared_impl",
"//printing",
"//services/device/wake_lock/power_save_blocker", # TODO: this requires a visibility patch to chromium src

This comment has been minimized.

@deepak1556

deepak1556 May 10, 2018

Member

Can you add this patch to libcc, since its required for this build.

@deepak1556

deepak1556 May 10, 2018

Member

Can you add this patch to libcc, since its required for this build.

This comment has been minimized.

@nornagon
@nornagon
@alexeykuzmin

Let's merge all needed refactorings separately.
There too many changes here for a single PR.

And could you please rebase your branch onto the latest master of e/e? There are no conflicts but it doesn't mean that everything will work after the merge.

@nornagon

This comment has been minimized.

Show comment
Hide comment
@nornagon

nornagon May 10, 2018

Contributor

Split out include path fixes to #12882 so it can be reviewed separately.

Contributor

nornagon commented May 10, 2018

Split out include path fixes to #12882 so it can be reviewed separately.

@nornagon

This comment has been minimized.

Show comment
Hide comment
@nornagon

nornagon May 10, 2018

Contributor

Split #ifdef changes out into #12884.

Contributor

nornagon commented May 10, 2018

Split #ifdef changes out into #12884.

@nornagon

This comment has been minimized.

Show comment
Hide comment
@nornagon

nornagon May 10, 2018

Contributor

All non-GN changes split out into separate PRs:
#12882 fixes include paths to not include vendor/ or node/deps
#12884 fixes some TOOLKIT_VIEWS-guarded code to also be OS_MACOSX-guarded, since the GN build defines TOOLKIT_VIEWS on macOS.
#12885 fixes a warning about an unused function that causes problems with the GN build

All the above PRs are required before this one can be merged.

Contributor

nornagon commented May 10, 2018

All non-GN changes split out into separate PRs:
#12882 fixes include paths to not include vendor/ or node/deps
#12884 fixes some TOOLKIT_VIEWS-guarded code to also be OS_MACOSX-guarded, since the GN build defines TOOLKIT_VIEWS on macOS.
#12885 fixes a warning about an unused function that causes problems with the GN build

All the above PRs are required before this one can be merged.

extra_source_filters += [
"*_views.cc",
"*_views.h",
"*\bviews/*",

This comment has been minimized.

@alexeykuzmin

alexeykuzmin May 10, 2018

Contributor

Why do we need those on Mac?

@alexeykuzmin

alexeykuzmin May 10, 2018

Contributor

Why do we need those on Mac?

This comment has been minimized.

@nornagon

nornagon May 10, 2018

Contributor

Because the code in there doesn't build on Mac, and also isn't used on Mac. In gyp-land, those filenames are filtered out by electron/brightray/filename_rules.gypi, but GN's filename filter list is shorter (see https://cs.chromium.org/chromium/src/build/config/BUILDCONFIG.gn?q=buildconfig&sq=package:chromium&dr&l=408)

@nornagon

nornagon May 10, 2018

Contributor

Because the code in there doesn't build on Mac, and also isn't used on Mac. In gyp-land, those filenames are filtered out by electron/brightray/filename_rules.gypi, but GN's filename filter list is shorter (see https://cs.chromium.org/chromium/src/build/config/BUILDCONFIG.gn?q=buildconfig&sq=package:chromium&dr&l=408)

@alexeykuzmin

Everything here seems to be new or obviously harmless, so shouldn't break anything.

@nornagon

This comment has been minimized.

Show comment
Hide comment
@nornagon

nornagon May 10, 2018

Contributor

@zcbenz want to join the party and also review this PR? :)

Contributor

nornagon commented May 10, 2018

@zcbenz want to join the party and also review this PR? :)

@zcbenz

zcbenz approved these changes May 15, 2018

👍

@zcbenz zcbenz merged commit 874af5c into electron:master May 15, 2018

10 checks passed

WIP ready for review
Details
ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
ci/circleci: electron-mas-x64 Your tests passed on CircleCI!
Details
ci/circleci: electron-osx-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@nornagon nornagon deleted the nornagon:build-gn branch Aug 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment