Closed Bug 1336429 Opened 7 years ago Closed 6 years ago

Support 3rd-party modules that use 'gn' as their build system

Categories

(Firefox Build System :: General, defect, P1)

Product:
Firefox Build System
Component:
General
Version:
45 Branch
Type:
defect

Tracking

(firefox59 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: jesup, Assigned: chmanchester)

References

(Depends on 2 open bugs)

Details

Attachments

(5 files, 3 obsolete files)

Bug 1336429 - Add the ability to build GN projects in the tree with mozbuild.
59 bytes, text/x-review-board-request
gps
: review+
Details
Fixes to gn processor for webrtc.org build
4.09 KB, patch
Details | Diff | Splinter Review
Parse libs in gn preprocessor
2.11 KB, patch
Details | Diff | Splinter Review
Hack to run gn.py on Windows
1.88 KB, patch
Details | Diff | Splinter Review
Changes to gn preprocessor for OS X and Android
4.48 KB, patch
Details | Diff | Splinter Review 
Upstream projects are starting to remove build support for anything but gn - webrtc.org code, I think libvpx, likely libyuv will if it hasn't, and skia (I'm told).  Others may follow suit.  In upstream webrtc.org, the current stable branch (57) has no gyp support; the last branch that had gyp was 55.  I can probably hack gyp support back in, but it will rapidly become a huge problem to import as it diverges from the last version that supported gyp.

We should look to take gn and process it into something we can use (unless we want to switch to using gn ourselves, or using gn as a backend to moz.build/etc).

This looks promising (JSON output from gn): https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/reference.md#Generic-JSON-Output and --ide=json https://chromium.googlesource.com/chromium/src/+/master/tools/gn/docs/reference.md#IDE-options
> We should look to take gn and process it into something we can use (unless we want to switch to using gn ourselves, or using gn as a backend to moz.build/etc).

I don't see this as particularly likely. The quickest way to make this work will be to treat it like GYP--we'd generate the JSON output during moz.build parsing and map it to moz.build constructs.
I've been the main lead for migrating the webrtc.org native code from GYP to GN and I can say it's a lot of work. It will surely be even more if you're going to try maintaining GYP files, especially as there's a lot of refactoring work going on moving things around. I highly discourage that option.

It's worth mentioning that we're developing a GN->Bazel converter internally, which is planned to be open sourced as soon we have something that's usable. I'm afraid that wouldn't be much of use for you though, since the moz.build format looks quite different (after reading at https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/How_Mozilla_s_build_system_works).

I'd recommend consuming the JSON output and make moz.build files out of it, althought be aware of that it's a significant amount of work if you're going to support all the Firefox platforms.
(In reply to Henrik Kjellander from comment #2)
> I've been the main lead for migrating the webrtc.org native code from GYP to
> GN and I can say it's a lot of work. It will surely be even more if you're
> going to try maintaining GYP files, especially as there's a lot of
> refactoring work going on moving things around. I highly discourage that
> option.

Thanks for the info! We're definitely not interested in maintaining a parallel build system for complex projects that are under active upstream development.

> I'd recommend consuming the JSON output and make moz.build files out of it,
> althought be aware of that it's a significant amount of work if you're going
> to support all the Firefox platforms.

We currently have support for consuming gyp files from moz.build files (which we originally wrote to build WebRTC). At build time we generate the gyp configuration for the current build target and translate that into the same data structures that our moz.build files produce. I suspect doing the same thing for GN will work OK, since that's effectively what GN is doing to generate Ninja files. We're certainly not going to try to write a general-purpose converter from GN to moz.build files.
bug 1340627 changes the skia import script to run gn to and parse its output to generate moz.build files, so it would be an informative place to start, I think.
Priority: -- → P1
Ted: can I ask you to pick this up, please? If you're too busy, please redirect to :chmanchester.
Assignee: nobody → ted
Ted's out, and will be until next week.

chmanchester: can I ask you to pick this up, and lean on Ted as required when he gets back?
Assignee: ted → cmanchester
Flags: needinfo?(cmanchester)
(In reply to Chris Cooper [:coop] from comment #6)
> Ted's out, and will be until next week.
> 
> chmanchester: can I ask you to pick this up, and lean on Ted as required
> when he gets back?

Sure, taking a look now.
Flags: needinfo?(cmanchester)
I'm assuming we don't want to require require a GN binary to build Firefox (GN is implemented in C++), so this can't work exactly like our GYP integration does today. One approach may be checking in json files produced by `gn gen --ide=json` for all the host/target os/cpu combinations we're intending to support for consumption by mozbuild, although this would be somewhat of a hassle at import time, and the files themselves are pretty large (>1mb each even after pruning out most of the subprojects we don't usually build, although if we filter down further to the keys we would end up consuming they would probably be a lot smaller). The good news is that the json format isn't a big departure from what we're getting from gyp, so once we got to this point integrating to moz.build would be pretty straightforward.

I also noticed that even for the webrtc revision in the tree definitions appear to have diverged between GN and GYP (for instance, I can't find the equivalent of "webrtc_lib" in the GN build files), so we'll need some additional changes to allow these GN definitions to build the webrtc source in our tree.
Yes, we want to avoid requiring a GN binary unless we require GN to build Firefox (which I doubt will happen). Requiring GN for developers touching 3rd party projects using GN is acceptable.

FWIW, this is essentially the same issue as Rust bindgen in that we need a new build dependency to generate code used by the build system. However, since GN is a build system that we don't use, that's a tougher pill to swallow. Bindgen uses LLVM and we use LLVM for Clang/Rust, so logically it's a reasonable dependency. GN as a dependency just sounds crazy.

Checking in generated JSON files is pretty annoying. Especially if we need one file per host/target os/cpu combination. Can you even generate all those combinations from the same machine or do you actually need to run things from different operating systems? If the latter, that's going to be a PITA for anyone touching a GN-based project.

The size of those JSON files is also a bit annoying. I'm sure they compress well. But if we can trim fat from what's checked in, that would be nice.
(In reply to Gregory Szorc [:gps] from comment #9)
> Checking in generated JSON files is pretty annoying. Especially if we need
> one file per host/target os/cpu combination. Can you even generate all those
> combinations from the same machine or do you actually need to run things
> from different operating systems? If the latter, that's going to be a PITA
> for anyone touching a GN-based project.
> 


In addition to host/target os/cpu combinations we'll need debug/opt.

Based on working with a local webrtc checkout we need to run things from a properly configured host machine to run `gn gen` successfully. Running GN with target_os="android" on my linux laptop worked once I set the right things in my .gclient file, but running with target_os="win" fails trying to find windows toolchain files. Running with target_os="mac" causes an assert in a gn file because cross compiling to mac isn't supported.
For the integration work to allow the webrtc GN build configuration to build webrtc in our tree, some of this needs to take place before concurrently with building GN support in mozbuild so we can verify the result. 

Dan, is this something you can help with?
Flags: needinfo?(dminor)
Depends on: 1382182
Of course, please let me know how I can help.

This might not be practical, but what if we programmatically generated moz.build files based upon the json output and checked those in? Maybe we could keep the mozilla specific stuff to a single moz.build that we maintain manually, and use generated files for the rest.

Some level of manual maintenance is going to be required whatever we do as we don't use the entire webrtc.org library and want to avoid building what we don't use.

Also, we should try to move anything under media/webrtc/signaling/* to moz.build as part of this work. I've filed Bug 1382182 for this, and that is something I can work on.
Flags: needinfo?(dminor)
(In reply to Gregory Szorc [:gps] from comment #9)
> Yes, we want to avoid requiring a GN binary unless we require GN to build
> Firefox (which I doubt will happen). Requiring GN for developers touching
> 3rd party projects using GN is acceptable.

I was expecting that we'd wind up requiring a GN binary because of the reasons chmanchester has noted. I originally tried to run gyp on update for WebRTC, but both tools want to know their build configuration up-front to spit out a concrete build plan, so they're not amenable to having that done ahead of time.

I thought about rewriting a gn parser, but I suspect that would cause us more headaches than it solves because we'd be trying to keep up matching gn features as well as trying to keep our build working.

My best plan was to basically build our own gn binaries in automation and have `mach bootstrap` pull those for developers.
(In reply to Dan Minor [:dminor] from comment #12)
> Of course, please let me know how I can help.
> 
> This might not be practical, but what if we programmatically generated
> moz.build files based upon the json output and checked those in? Maybe we
> could keep the mozilla specific stuff to a single moz.build that we maintain
> manually, and use generated files for the rest.
> 
> Some level of manual maintenance is going to be required whatever we do as
> we don't use the entire webrtc.org library and want to avoid building what
> we don't use.
> 
> Also, we should try to move anything under media/webrtc/signaling/* to
> moz.build as part of this work. I've filed Bug 1382182 for this, and that is
> something I can work on.

Thank you, Dan. I think we need to be able to run `gn gen` in the tree and have that generate build instructions for how we want to build webrtc. It would be particularly helpful if we had usable gyp files at the same time to compare.
(In reply to Chris Manchester (:chmanchester) from comment #14)
> (In reply to Dan Minor [:dminor] from comment #12)
> > Of course, please let me know how I can help.
> > 
> > This might not be practical, but what if we programmatically generated
> > moz.build files based upon the json output and checked those in? Maybe we
> > could keep the mozilla specific stuff to a single moz.build that we maintain
> > manually, and use generated files for the rest.
> > 
> > Some level of manual maintenance is going to be required whatever we do as
> > we don't use the entire webrtc.org library and want to avoid building what
> > we don't use.
> > 
> > Also, we should try to move anything under media/webrtc/signaling/* to
> > moz.build as part of this work. I've filed Bug 1382182 for this, and that is
> > something I can work on.
> 
> Thank you, Dan. I think we need to be able to run `gn gen` in the tree and
> have that generate build instructions for how we want to build webrtc. It
> would be particularly helpful if we had usable gyp files at the same time to
> compare.

Just to make sure I follow, you want me to make sure it's possible to run gn gen from media/webrtc/trunk and get useful results?

We're currently using manually updated versions of the gyp files to build the webrtc.org code in tree, so those are available for comparison. We added (some of?) the BUILD.gn files in the last webrtc.org update, but since we weren't using them to build, I'm not sure if we have a complete set. I'll have a look next week.

Also, for what it's worth, it appears that we currently build libyuv using gyp, and it has BUILD.gn files checked in. That might be an simpler place to start than the webrtc.org code.
(In reply to Dan Minor [:dminor] from comment #15)
> (In reply to Chris Manchester (:chmanchester) from comment #14)
> > (In reply to Dan Minor [:dminor] from comment #12)
> > > Of course, please let me know how I can help.
> > > 
> > > This might not be practical, but what if we programmatically generated
> > > moz.build files based upon the json output and checked those in? Maybe we
> > > could keep the mozilla specific stuff to a single moz.build that we maintain
> > > manually, and use generated files for the rest.
> > > 
> > > Some level of manual maintenance is going to be required whatever we do as
> > > we don't use the entire webrtc.org library and want to avoid building what
> > > we don't use.
> > > 
> > > Also, we should try to move anything under media/webrtc/signaling/* to
> > > moz.build as part of this work. I've filed Bug 1382182 for this, and that is
> > > something I can work on.
> > 
> > Thank you, Dan. I think we need to be able to run `gn gen` in the tree and
> > have that generate build instructions for how we want to build webrtc. It
> > would be particularly helpful if we had usable gyp files at the same time to
> > compare.
> 
> Just to make sure I follow, you want me to make sure it's possible to run gn
> gen from media/webrtc/trunk and get useful results?

Yes, exactly.

> 
> We're currently using manually updated versions of the gyp files to build
> the webrtc.org code in tree, so those are available for comparison. We added
> (some of?) the BUILD.gn files in the last webrtc.org update, but since we
> weren't using them to build, I'm not sure if we have a complete set. I'll
> have a look next week.

Ok, I guess that explains the drift between gn and gyp as seen in our tree right now.

> 
> Also, for what it's worth, it appears that we currently build libyuv using
> gyp, and it has BUILD.gn files checked in. That might be an simpler place to
> start than the webrtc.org code.

I see, that sounds like a reasonable place to start, I'll take a look.

Thanks Dan!
I've put the patches to let us run gn gen from media/webrtc/trunk on Google Drive. The first patch adds missing files and is rather large. Hopefully we can prune it down later. The second patch disables stuff that we shouldn't need right away in order to get gn gen to run.

Part 1 - https://drive.google.com/a/mozilla.com/file/d/0B9-jnKuqA3bJYWl5UG93a1g3Y1E/view?usp=sharing
Part 2 - https://drive.google.com/a/mozilla.com/file/d/0B9-jnKuqA3bJem5ST21hZU9sUzA/view?usp=sharing
Resulting ninja files - https://drive.google.com/a/mozilla.com/file/d/0B9-jnKuqA3bJYWl5UG93a1g3Y1E/view?usp=sharing

This was a hack and slash approach to get things running, we'll have to go over this with more care once we're further along. Please let me know if there's any problem accessing the files, first time I'm using Google Drive for this :)
Thanks Dan. I'm working on getting GN to build in our automation, but I'll take a look at these next week.
(In reply to Dan Minor [:dminor] from comment #17)
> patches to let us run gn gen from media/webrtc/trunk on Google Drive

The patches aren't accessible outside of MoCo (presents login form). Is it intentional?
(In reply to Jan Beich from comment #19)
> (In reply to Dan Minor [:dminor] from comment #17)
> > patches to let us run gn gen from media/webrtc/trunk on Google Drive
> 
> The patches aren't accessible outside of MoCo (presents login form). Is it
> intentional?

Unfortunately the options seem to be share with everyone at Mozilla or add people individually. I've attempted to add you individually. I'm sorry about the use of Google Drive, one of the patch files was too large to attach to this bug, which would have been my first choice. I was attempting to do something quick for Chris prior to leaving for vacation. I plan to redo these patches with more care in the future.
Dan, I've posted the patches I was referring to earlier. With these, the patches from comment 17, and the patches for bug 1382182 applied I can run |./mach configure| and get a plausible result if I also revert the deletion of common.gypi in one of the patches from comment 17 (gyp projects other than webrtc rely on that file somehow). Trying to build with this output, some directories succeed and others fail due to missing sources, so there's at least a little ways to go here.

Let me know if you have any questions or issues with this from here! Thanks.
Blocks: 1376873
Blocks: 1393119
Comment on attachment 8897668 [details]
Bug 1336429 - Add the ability to build GN projects in the tree with mozbuild.

https://reviewboard.mozilla.org/r/168940/#review178064

::: python/mozbuild/mozbuild/frontend/gn_processor.py:42
(Diff revision 1)
> +    out_dir = mozpath.join(output, 'gn-output')
> +    gen_args = [
> +        config.substs['GN'], 'gen', out_dir, gn_args, '--ide=json',
> +    ]
> +    print("Running \"%s\"" % ' '.join(gen_args))
> +    subprocess.check_call(gen_args, cwd=srcdir)

Please use check_output with stderr=subprocess.STDOUT here so the user can see any gn errors that result from their modifications to the file.

::: python/mozbuild/mozbuild/frontend/gn_processor.py:72
(Diff revision 1)
> +
> +    # Process all targets from the given gn project and its dependencies.
> +    for target_fullname in all_deps:
> +        spec = targets[target_fullname]
> +
> +        if spec['type'] not in ('static_library', 'shared_library', 'executable'):

Please add 'source_set' to this list.

::: python/mozbuild/mozbuild/frontend/gn_processor.py:89
(Diff revision 1)
> +        context.add_source(mozpath.join(srcdir, target_path, 'BUILD.gn'))
> +
> +        # Remove leading 'lib' from the target_name if any, and use as
> +        # library name.
> +        name = target_name
> +        if spec['type'] in ('static_library', 'shared_library'):

Please add 'source_set' to this list.

::: python/mozbuild/mozbuild/frontend/gn_processor.py:154
(Diff revision 1)
> +                resolved = mozpath.abspath(mozpath.join(config.topsrcdir, include[1:]))
> +            else:
> +                resolved = mozpath.abspath(mozpath.join(srcdir, include))
> +            if not os.path.exists(resolved):
> +                continue
> +            include = '/%s/%s' % (project_relsrcdir, include)

We need to be able to break out of the webrtc subdirectory to include things like libyuv which we build elsewhere in tree.

Something like:

            if include.startswith('/'):
                include = '/%s/%s' % (config.topsrcdir, include)
            else:
                include = '/%s/%s' % (project_relsrcdir, include)
Comment on attachment 8897668 [details]
Bug 1336429 - Add the ability to build GN projects in the tree with mozbuild.

https://reviewboard.mozilla.org/r/168940/#review178070

::: python/mozbuild/mozbuild/frontend/gn_processor.py:152
(Diff revision 1)
> +            # filtered out by trying to find them in topsrcdir.
> +            if include.startswith('/'):
> +                resolved = mozpath.abspath(mozpath.join(config.topsrcdir, include[1:]))
> +            else:
> +                resolved = mozpath.abspath(mozpath.join(srcdir, include))
> +            if not os.path.exists(resolved):

Please add a warning here that the include is being dropped.
I'm pretty close to getting the gn build working. Thanks Chris!
These are my local fixes to the gn processor.
I've pushed my webrtc.org build patches to Bug 1393119. With those patches, the patches applied to this bug, and a copy of the gn executable available under /media/webrtc/trunk/buildtools/linux64 I'm able to get a complete build.

Unfortunately, it crashes immediately as soon as it touches webrtc code. I'm guessing a mismatch between preprocessor defines and/or compiler options, but I haven't done much investigation yet.
Dan, I updated the patches here with your feedback as well as a fix to how I was passing in arguments to GN that might be related to the issues you're seeing in comment 28.
(In reply to Chris Manchester (:chmanchester) from comment #31)
> Dan, I updated the patches here with your feedback as well as a fix to how I
> was passing in arguments to GN that might be related to the issues you're
> seeing in comment 28.

Chris, thank you for the rebase. With your fix in place, I was able to make an appear.in call, so things seem to be working properly now.
I spoke too soon, I'm back to crashes and test failures after I rebased on top of central. It's possible I got a good build this morning but something caused it to subsequently break. It's also possible that I forgot to do a clobber and got some kind of "frankenbuild" which happened to work, and it's also possible that I simply forgot to update to tip and built with gyp rather than gn. I'll investigate further tomorrow.
I'm tracking down preprocessor differences between the gyp and gn builds. Here is a list:
-DCOMPONENT_BUILD (added)
-DENABLE_AUTOMATION=1 (removed)
-DENABLE_BACKGROUND=1 (removed)
-DENABLE_CAPTIVE_PORTAL_DETECTION=1 (removed)
-DENABLE_CONFIGURATION_POLICY (removed)
-DENABLE_EGLIMAGE=1 (removed)
-DENABLE_EXTENSIONS=1 (removed)
-DENABLE_GPU=1 (removed)
-DENABLE_INPUT_SPEECH (removed)
-DENABLE_NOTIFICATIONS (removed)
-DENABLE_ONE_CLICK_SIGNIN (removed)
-DENABLE_PLUGIN_INSTALLATION=1 (removed)
-DENABLE_PRINTING=1 (removed)
-DENABLE_PROTECTOR_SERVICE=1 (removed)
-DENABLE_REMOTING=1 (removed)
-DENABLE_SESSION_SERVICE=1 (removed)
-DENABLE_TASK_MANAGER=1 (removed)
-DENABLE_THEMES=1 (removed)
-DENABLE_WEBRTC=1 (removed)
-DENABLE_WEB_INTENTS=1 (removed)
-DFIELDTRIAL_TESTING_ENABLED (added)
-DFULL_SAFE_BROWSING (added)
-DGTK_DISABLE_SINGLE_INCLUDES=1 (removed)
-DRTC_DISABLE_VP9 (added)
-DSAFE_BROWSING_CSD (added)
-DSAFE_BROWSING_DB_LOCAL (added)
-DUSE_AURA=1 (added)
-DUSE_CAIRO=1 (added)
-DUSE_GLIB=1 (added)
-DUSE_LIBJPEG_TURBO=1 (removed)
-DUSE_NSS=1 (removed)
-DUSE_NSS_CERTS=1 (added)
-DUSE_PANGO=1 (added)
-DUSE_SKIA=1 (removed)
-DUSE_UDEV (added)
-DUSE_X11=1 (added)
-DV8_DEPRECATION_WARNINGS (added)
-D_GLIBCXX_DEBUG=1 (added)
-D_ISOC99_SOURCE=1 (removed)
-D_LARGEFILE64_SOURCE (added)
-D_LARGEFILE_SOURCE (added)
-D__STDC_CONSTANT_MACROS (added)

Some of these are from differences between the webrtc gyp and gn files, but it looks like some of these (the -DENABLE_* ones for instance) should be inherited from elsewhere in the build system but are not being set. It's unlikely that these are causing the crashes I'm seeing, but it would simplify my work if we could rule them out. Chris, do you mind checking if these should be passed in from elsewhere in the build system? Thanks!
Flags: needinfo?(cmanchester)
Dan and I chatted about this on irc and it looks like it's sorted out.
Flags: needinfo?(cmanchester)
I've uploaded the patches I have to generate moz.build files from a gn config rather than require gn to build. With these and the updated patches from bug 1393119 I'm able to produce a build that passes the media mochitests.

The procedure for now is to run ./mach configure with `ac_add_options --build-backends=GnConfigGen` on a machine that has gn available, which will generate a json file (whose name will be derived from the arguments passed to `gn gen`) in $topobjdir/gn-output`, copy the resulting file to media/webrtc/gn-configs, and then run ./mach configure with `ac_add_options --build-backends=GnMozbuildWriter`, which will generate moz.build files under media/webrtc/trunk. 

With multiple configs under media/webrtc/gn-configs the mozbuild writer will make an effort to factor things to avoid duplication, but this hasn't been tested thoroughly yet. I'm also thinking about splitting out the mozbuild generator to a separate mach command before landing, this can and probably should be done independently of an individual build.
I'd script the functionality as a `mach vendor` sub-command. `mach vendor webrtc-gn` or something?
We need some additional os libraries for desktop capture support on Windows, parsing 'libs' into OS_LIBS seems to do the trick, although I haven't tested this patch back on the Linux build yet.
This is to workaround a problem where if we try to run gn with the buildtools packaged version of Python we seem to hit a missing symbol in a .dll when we import stuff from the depot_tools version of the Python libs, and get a helpful error message like %1 is not a Windows program.
I did a plain opt build (instead of opt+debug) and the build completes, the tests run, I see a few failures that I'll take care of later. I've pushed my updated patches to Bug 1393119. We need to get things working without the ugly hack I attached above, and we do need to get opt+debug builds working properly.

Also, I almost always forget to copy the gn output from the objdir to media/webrtc and waste time wondering why my changes didn't make any difference, so it would be great if that step was done automatically or would not be necessary :)
(In reply to Dan Minor [:dminor] from comment #46)
> I did a plain opt build (instead of opt+debug) and the build completes, the
> tests run, I see a few failures that I'll take care of later. I've pushed my
> updated patches to Bug 1393119. We need to get things working without the
> ugly hack I attached above, and we do need to get opt+debug builds working
> properly.

I was able to get "gn gen" to complete with a gn.exe from our toolchain build (with "./mach artifact toolchain --from-build win32-gn") and commenting out the block that runs vs_toolchain.py from build/config/win/visual_studio_version.gni.

> 
> Also, I almost always forget to copy the gn output from the objdir to
> media/webrtc and waste time wondering why my changes didn't make any
> difference, so it would be great if that step was done automatically or
> would not be necessary :)

We can't assume these are happening on the same machine, so I don't know how much ultimate benefit there is to this, but in the mean time I'll push an update to check for configs in the objdir as well :)
I was able to get an --enable-debug --enable-optimize build working by removing "_DEBUG" from the defines added by /media/webrtc/trunk/build/config/BUILD.gn for debug builds. Apparently, and I couldn't find documentation on this point, if you pass -MD but also define _DEBUG, msvc will link to the debug runtime anyway, which causes a mismatch when it tries to link to the rest of our code that wasn't compiled with _DEBUG (which is the case for --enable-debug builds with MOZ_MEMORY defined).

I had to update my patches on top of some recent changes as well, I'll upload those updates shortly.
(In reply to Chris Manchester (:chmanchester) from comment #47)
> (In reply to Dan Minor [:dminor] from comment #46)
> > I did a plain opt build (instead of opt+debug) and the build completes, the
> > tests run, I see a few failures that I'll take care of later. I've pushed my
> > updated patches to Bug 1393119. We need to get things working without the
> > ugly hack I attached above, and we do need to get opt+debug builds working
> > properly.
> 
> I was able to get "gn gen" to complete with a gn.exe from our toolchain
> build (with "./mach artifact toolchain --from-build win32-gn") and
> commenting out the block that runs vs_toolchain.py from
> build/config/win/visual_studio_version.gni.
> 

Oh ok, I didn't realize that you had that working. That would have been easier!

> > 
> > Also, I almost always forget to copy the gn output from the objdir to
> > media/webrtc and waste time wondering why my changes didn't make any
> > difference, so it would be great if that step was done automatically or
> > would not be necessary :)
> 
> We can't assume these are happening on the same machine, so I don't know how
> much ultimate benefit there is to this, but in the mean time I'll push an
> update to check for configs in the objdir as well :)

I think the more common case will be on the same machine, trying to fix gn files while doing a webrtc.org update. I know we've discussed having a Try job to help with generating files for other platforms, would that generate intermediate json files or the final moz.build files?
I tried adding cflags_mozilla to the gn files, but I was unable to get it to serialize the new variable to the json files. Rather than spend a bunch of time, I switched to using cflags and a whitelist of flags to pass through to moz.build. I also added a few missing imports.

The other change is to special case the parsing of libs to pass in -framework libname on OS X. These might be redundant with what we're inheriting from the rest of the build system but it doesn't see to be doing any harm.
Comment on attachment 8897668 [details]
Bug 1336429 - Add the ability to build GN projects in the tree with mozbuild.

https://reviewboard.mozilla.org/r/168940/#review200516

::: python/mozbuild/mozbuild/gn_processor.py:148
(Diff revision 6)
> +    all_deps = set()
> +
> +    def add_target_deps(target):
> +        all_deps.add(target)
> +        for dep in targets[target]['deps']:
> +            all_deps.add(dep)

It is possible to hit circular dependencies, please replace
    all_deps.add(dep)
with
    if dep not in all_deps:
Comment on attachment 8897668 [details]
Bug 1336429 - Add the ability to build GN projects in the tree with mozbuild.

https://reviewboard.mozilla.org/r/168940/#review201984

::: python/mozbuild/mozbuild/gn_processor.py:135
(Diff revision 6)
> +        'OS_TARGET',
> +        'HOST_CPU_ARCH',
> +        'CPU_ARCH',
> +    )
> +
> +    mozbuild_args = {k: config.substs[k] for k in gn_mozbuild_vars}

I get a key error for MOZ_DEBUG in opt builds with no debug symbols.

I used:

mozbuild_args = {k: config.substs.get(k, False) for k in gn_mozbuild_vars}

as a workaround.
Comment on attachment 8897667 [details]
Bug 1336429 - Detect GN in configure.

https://reviewboard.mozilla.org/r/168938/#review204462

::: toolkit/moz.configure:551
(Diff revision 6)
>  # ==============================================================
>  
>  check_prog('TAR', ('gnutar', 'gtar', 'tar'))
>  check_prog('UNZIP', ('unzip',))
>  check_prog('ZIP', ('zip',))
> +check_prog('GN', ('gn',))

We should allow this to be missing.
Attachment #8897667 - Attachment is obsolete: true
Comment on attachment 8897668 [details]
Bug 1336429 - Add the ability to build GN projects in the tree with mozbuild.

https://reviewboard.mozilla.org/r/168940/#review207042

While a large amount of code, this seems pretty straightforward. The overall design looks pretty solid and I don't see any major issues hindering eventual r+.

I glanced over some parts of code. So I'll need to do another review once all the quirks are worked out. Great work!

::: python/mozbuild/mozbuild/gn_processor.py:77
(Diff revision 8)
> +            self.write('\n')
> +            self.write(self.indent + key)
> +            self.write(' += [\n    ' + self.indent)
> +            self.write((',\n    ' + self.indent).join(self.mb_serialize(v) for v in value))
> +            self.write('\n')
> +            self.write_ln(']')

Thank you for taking the time to ensure the written moz.build files look nice.

::: python/mozbuild/mozbuild/gn_processor.py:207
(Diff revision 8)
> +        else:
> +            context_attrs['PROGRAM'] = name.decode('utf-8')

I think this should assert `spec['type']`'s value here as a defense against additional types being encountered.

::: python/mozbuild/mozbuild/gn_processor.py:225
(Diff revision 8)
> +            ext = mozpath.splitext(f)[-1]
> +            extensions.add(ext)
> +            src = '%s/%s' % (project_relsrcdir, f)
> +            if ext == '.h':
> +                continue
> +            if ext == '.def':

elif

::: python/mozbuild/mozbuild/gn_processor.py:230
(Diff revision 8)
> +            if ext == '.def':
> +                context_attrs['SYMBOLS_FILE'] = src
> +            elif ext != '.S' and src not in non_unified_sources:
> +                unified_sources.append('/%s' % src)
> +            else:
> +                sources.append('/%s' % src)

Again, should we validate the extension is in a known set? Although I suppose moz.build will barf on unknown extension type.

::: python/mozbuild/mozbuild/gn_processor.py:233
(Diff revision 8)
> +                unified_sources.append('/%s' % src)
> +            else:
> +                sources.append('/%s' % src)
> +            # The Mozilla build system doesn't use DEFINES for building
> +            # ASFILES.
> +            if ext == '.s':

What about `.S`?

::: python/mozbuild/mozbuild/gn_processor.py:413
(Diff revision 8)
> +
> +    for relsrcdir, configs in configs_by_dir.items():
> +        target_srcdir = mozpath.join(config.topsrcdir, relsrcdir)
> +        try:
> +            os.makedirs(target_srcdir)
> +        except OSError:

This can swallow an exception for legitimate failures. You want this to ignore ignore errors on `e.errno == errno.EEXIST`.

Or you could use `mozbuild.util.mkdir()`.

::: python/mozbuild/mozbuild/gn_processor.py:513
(Diff revision 8)
> +        gn_out = json.load(fh)
> +        gn_out = filter_gn_config(gn_out, config, sandbox_variables,
> +                                  input_variables)
> +
> +
> +    os.remove(gn_config_file)

It feels like this may want to belong in a `finally` block.

::: python/mozbuild/mozbuild/gn_processor.py:517
(Diff revision 8)
> +
> +    os.remove(gn_config_file)
> +
> +    gn_out_file = mozpath.join(out_dir, gn_arg_string + '.json')
> +    with open(gn_out_file, 'w') as fh:
> +        json.dump(gn_out, fh, indent=4)

Add `sort_keys=True` for stability.

::: python/mozbuild/mozbuild/gn_processor.py:526
(Diff revision 8)
> +            if not gn_binary:
> +                raise Exception("The GN program must be present to generate GN configs.")

Hmmm. It feels like this check should be in configure. But that assumes this backend is enabled in configure. Not sure if we intend to do things that way or manually run the backend via `mach build-backend`.

::: python/mozbuild/mozbuild/gn_processor.py:543
(Diff revision 8)
> +            gn_config_files = glob.glob(mozpath.join(obj.srcdir, 'gn-configs', '*.json'))
> +            if not gn_config_files:
> +                gn_config_files = glob.glob(mozpath.join(obj.topobjdir, 'gn-output', '*.json'))

It kinda feels weird to me that we look in both locations by default. This deserves a comment explaining the behavior.

::: python/mozbuild/mozbuild/test/backend/data/gn-processor/gn-configs/x64_False_x64_linux.json:3
(Diff revision 8)
> +{
> +  "mozbuild_args": {
> +    "HOST_CPU_ARCH": "x86_64", 

If you use `separators=(',', ': ')` when calling `json.dump()`, it avoids the end of line whitespace. This is also the default behavior in Python 3. Might as well change it to avoid the churn when we port to Python 3.

::: python/mozbuild/mozbuild/test/backend/test_gn_processor.py:35
(Diff revision 8)
> +    def setUp(self):
> +        self._backup_dir = tempfile.mkdtemp(prefix='mozbuild')
> +        self._gn_dir = os.path.join(os.path.dirname(__file__),
> +                                    'data', 'gn-processor', 'trunk')
> +        shutil.copytree(os.path.join(self._gn_dir),
> +                        os.path.join(self._backup_dir, 'trunk'))
> +
> +    def tearDown(self):
> +        shutil.rmtree(self._gn_dir)
> +        shutil.copytree(os.path.join(self._backup_dir, 'trunk'),
> +                        self._gn_dir)
> +        shutil.rmtree(self._backup_dir)

These are missing a call to their implementations in the parent class.
Attachment #8897668 - Flags: review?(gps)
Comment on attachment 8897668 [details]
Bug 1336429 - Add the ability to build GN projects in the tree with mozbuild.

https://reviewboard.mozilla.org/r/168940/#review207042

> Again, should we validate the extension is in a known set? Although I suppose moz.build will barf on unknown extension type.

I should mention a lot of this was based on the equivalent `gyp_reader` code. I'll a comment to that effect. Letting moz.build to the validation might be more reliable.

> What about `.S`?

Not sure. This was added to `gyp` for the NSS build, where this was the only case we cared about.

> It feels like this may want to belong in a `finally` block.

Not sure about that... if `filter_gn_config` ever does fail the raw project.json might be helpful for debugging.

> Hmmm. It feels like this check should be in configure. But that assumes this backend is enabled in configure. Not sure if we intend to do things that way or manually run the backend via `mach build-backend`.

Right, I'm expecting `mach build-backend` might be the most convenient way to go about this.
Attachment #8930627 - Attachment is obsolete: true
In this try job: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c94f287edeb496663947bc5aabf1cc4508f334d
only the android .json files contain "-mpfu=neon" but we're generating a moz.build file (webrtc/modules/audio_mixer/audio_mixer_impl_gn/moz.build) that contains:

if not CONFIG["MOZ_DEBUG"]:

    CXXFLAGS += [
        "-mfpu=neon"
    ]
(the json files are checked in under media/webrtc/gn-configs)
(In reply to Dan Minor [:dminor] from comment #64)
> In this try job:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=6c94f287edeb496663947bc5aabf1cc4508f334d
> only the android .json files contain "-mpfu=neon" but we're generating a
> moz.build file (webrtc/modules/audio_mixer/audio_mixer_impl_gn/moz.build)
> that contains:
> 
> if not CONFIG["MOZ_DEBUG"]:
> 
>     CXXFLAGS += [
>         "-mfpu=neon"
>     ]

I added a fix for this and a unit test in comment 66, with it I'm only getting '-mfpu=neon' under 'OS_TARGET' == 'Android'.
(In reply to Chris Manchester (:chmanchester) from comment #67)
> (In reply to Dan Minor [:dminor] from comment #64)
> > In this try job:
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=6c94f287edeb496663947bc5aabf1cc4508f334d
> > only the android .json files contain "-mpfu=neon" but we're generating a
> > moz.build file (webrtc/modules/audio_mixer/audio_mixer_impl_gn/moz.build)
> > that contains:
> > 
> > if not CONFIG["MOZ_DEBUG"]:
> > 
> >     CXXFLAGS += [
> >         "-mfpu=neon"
> >     ]
> 
> I added a fix for this and a unit test in comment 66, with it I'm only
> getting '-mfpu=neon' under 'OS_TARGET' == 'Android'.

great, thank you!
Comment on attachment 8897668 [details]
Bug 1336429 - Add the ability to build GN projects in the tree with mozbuild.

https://reviewboard.mozilla.org/r/168940/#review211238
Attachment #8897668 - Flags: review?(gps) → review+
I think I've found another moz.build code gen bug in this try push [1].

If you look at [2], the desktop_capture module is defined for x64 Linux and there are Linux specific files in [3]. But the generated media/webrtc/moz.build trunk file is only including desktop_capture for windows builds [4].

[1] https://treeherder.mozilla.org/logviewer.html#?job_id=150228121&repo=try&lineNumber=31703
[2] https://hg.mozilla.org/try/file/b6482d257974/media/webrtc/gn-configs/x64_True_x64_linux.json#l5715
[3] https://hg.mozilla.org/try/file/b6482d257974/media/webrtc/trunk/webrtc/modules/desktop_capture/desktop_capture_gn/moz.build
[4] https://hg.mozilla.org/try/file/b6482d257974/media/webrtc/trunk/moz.build#l92
Flags: needinfo?(cmanchester)
I have a fix I'll post shortly. Sorry about the churn here.
Flags: needinfo?(cmanchester)
One last problem (all of the other builds are green!) with android x86 [1]: The generated media/webrtc/trunk/moz.build file [2] does not contain any Android section at all, compare to [3]. I think the problem is that on ARM we include a set of files for NEON, and on x86 we include a set of files for SSE2 and the code generator needs to handle creating if blocks based upon both target os and processor in this case.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=a5a91985e36f6d05692e8913a860392a4cf1f0e7
[2] https://hg.mozilla.org/try/file/bfeb6f091b79/media/webrtc/trunk/moz.build
[3] https://hg.mozilla.org/try/file/d8aeaeaebf29/media/webrtc/trunk/moz.build
Flags: needinfo?(cmanchester)
Yep, the code was assuming OS_TARGET was enough to filter dirs. I have a fix I tested on try with our various android builds, I'll push it up in a moment.
Flags: needinfo?(cmanchester)
Please have a look at this try run [1], I believe this line [2] in media/webrtc/moz.build should be

if CONFIG["CPU_ARCH"] != "x86" and CONFIG["OS_TARGET"] == "Android":

as we're ending up trying to build with NEON on x86 again. As far as I can tell, the generated json files are fine, so I think this is another code generation problem.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c2916cf464b29b43c7b0f58eb1afe082bf18c15
[2] https://hg.mozilla.org/try/file/144d6e0bb509/media/webrtc/trunk/moz.build#l112
Pushed by cmanchester@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ac7cab8746a2
Add the ability to build GN projects in the tree with mozbuild. r=gps
https://hg.mozilla.org/mozilla-central/rev/ac7cab8746a2
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
No longer depends on: 1387159
Product: Core → Firefox Build System
Depends on: 1466254
Depends on: 1526287
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: