1200311 - Build the gtest libxul during the build

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Chris Manchester (limited bugmail, email directly)]

Bug 992983 will move building the gtest libxul from make check to the test packaging step. We want to do it as part of the main build, which might help us deal with bug 1055224 as well.

Comment 1

[Chris Manchester (limited bugmail, email directly)]

Attachment: Bug 1200311 - Build the gtest libxul during the compile tier instead of package-tests.

Review commit: https://reviewboard.mozilla.org/r/104268/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/104268/

Comment 2

[Mike Hommey [:glandium]]

I don't think building gtest during mach build is a good idea. There is a reason it was moved to build during make check, and that reason still applies: we don't want to incur the cost of mach build having to link *two* libxul. Even if they were linking in parallel (which they aren't because one depends on the other at the moment), it would incur a memory usage problem, especially on PGO builds.

Comment 3

[Mike Hommey [:glandium]]

Mmmm reading the patch, I see you're enabling that on automation only, which makes it less bad, I guess.

Comment 4

[(not currently active) Ted Mielczarek]

Yeah, in automation this has been hacky for a long time. We're effectively doing linking in the package-tests tier. We might as well be doing the link in the compile tier where we link everything else. This still might wind up not being great--we might wind up linking both copies of libxul at the same time and starving them. If that turns out to be a problem we could make the gtest libxul force a dependency on the normal libxul to serialize them.

Comment 5

[Chris Manchester (limited bugmail, email directly)]

In defense of this approach, package-tests is currently a long pole for automation steps on Linux and Windows, so this change could unlock some potential for speedups there.

Comment 6

[(not currently active) Ted Mielczarek]

Comment on attachment 8826364 [details]
Bug 1200311 - Build the gtest libxul during the compile tier instead of package-tests.

https://reviewboard.mozilla.org/r/104268/#review105054

Sorry for the lag, I had to find time to re-read all these Makefiles and remember how they work. This looks good, thanks!

::: moz.configure:162
(Diff revision 1)
>  
>  set_config('BUILD_BACKENDS', build_backends)
>  
> +# Determine whether to build the gtest xul. This happens in automation
> +# on Desktop platforms with the exception of Windows PGO, where linking
> +# xul-gtest.dll takes too long (bug 1028035).

The bug mentioned here got duped to another bug and neither of them are particularly informative about this. I think you should just leave this out for now. (Also it'd be interesting to do a try push and see what the impact on build times is nowadays, since PGO builds are much faster.)

::: moz.configure:170
(Diff revision 1)
> +def build_gtest(pgo, build_project, target, automation):
> +    if (automation and build_project == 'browser' and
> +        not (pgo and target.os == 'WINNT')):
> +        return True
> +
> +set_config('BUILD_GTEST', build_gtest)

It feels pedantic, but something slightly more descriptive might be nice, like `BUILD_GTEST_DURING_BUILD`? If a developer runs `mach gtest` locally we should still build gtest when that happens.

::: toolkit/library/Makefile.in:19
(Diff revision 1)
>  
> +ifdef BUILD_GTEST
> +target:: $(DIST)/bin/dependentlibs.list.gtest
> +endif
> +
> +$(DIST)/bin/dependentlibs.list.gtest: dependentlibs.list

I think you could move this to `GENERATED_FILES` in the moz.build while you're at it, no? The normal `dependentlibs.list` is already there:
https://dxr.mozilla.org/mozilla-central/rev/1d025ac534a6333a8170a59a95a8a3673d4028ee/toolkit/library/moz.build#381

You could just add a new function in the same file that does this replacement. (That'd also let you avoid the `target::` bit above.)

::: toolkit/library/gtest/Makefile.in:13
(Diff revision 1)
> -LINK_GTEST = 1
> +BUILD_GTEST = 1
>  endif
>  
> -ifndef LINK_GTEST
> +ifndef BUILD_GTEST
>  # Force to not include backend.mk unless LINK_GTEST is defined.
>  # Not including backend.mk makes traversing this directory do nothing.

It'd be nice to find a cleaner solution to this at some point. I guess we'd need some concept of "directories which we don't build by default". Actually now that I say that it doesn't sound terribly hard, I filed bug 1337008 about that idea.

Comment 7

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8826364 [details]
Bug 1200311 - Build the gtest libxul during the compile tier instead of package-tests.

https://reviewboard.mozilla.org/r/104268/#review105054

> I think you could move this to `GENERATED_FILES` in the moz.build while you're at it, no? The normal `dependentlibs.list` is already there:
> https://dxr.mozilla.org/mozilla-central/rev/1d025ac534a6333a8170a59a95a8a3673d4028ee/toolkit/library/moz.build#381
> 
> You could just add a new function in the same file that does this replacement. (That'd also let you avoid the `target::` bit above.)

This is a little tricky, because of how we do |./mach gtest|, but this should be fine if we just always generate dependentlibs.list.gtest alongside dependentlibs.list.

Comment 8

[Chris Manchester (limited bugmail, email directly)]

Attachment: Bug 1200311 - Move dependentlibs.list.gtest generation to GENERATED_FILES.

Review commit: https://reviewboard.mozilla.org/r/110196/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/110196/

Comment 9

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8826364 [details]
Bug 1200311 - Build the gtest libxul during the compile tier instead of package-tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104268/diff/1-2/

Comment 10

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8826364 [details]
Bug 1200311 - Build the gtest libxul during the compile tier instead of package-tests.

https://reviewboard.mozilla.org/r/104268/#review105054

> The bug mentioned here got duped to another bug and neither of them are particularly informative about this. I think you should just leave this out for now. (Also it'd be interesting to do a try push and see what the impact on build times is nowadays, since PGO builds are much faster.)

Testing this on try for TC builds, the builds succeed, but it's a big slowdown: https://treeherder.mozilla.org/perf.html#/graphs?series=%5Btry,45109a919aceb8217a8e98fcfbb669ee989701f6,1,2%5D&selected=%5Btry,45109a919aceb8217a8e98fcfbb669ee989701f6,167512,74924798%5D

Comment 11

[Michael Shal [:mshal]]

Comment on attachment 8834176 [details]
Bug 1200311 - Move dependentlibs.list.gtest generation to GENERATED_FILES.

https://reviewboard.mozilla.org/r/110196/#review111612

Looks good!

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9a1db16ec7c2
Move dependentlibs.list.gtest generation to GENERATED_FILES. r=mshal
https://hg.mozilla.org/integration/autoland/rev/87afb64a7e5a
Build the gtest libxul during the compile tier instead of package-tests. r=ted

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9a1db16ec7c2
https://hg.mozilla.org/mozilla-central/rev/87afb64a7e5a

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora approval on this when you get a chance.

Comment 15

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8826364 [details]
Bug 1200311 - Build the gtest libxul during the compile tier instead of package-tests.

Approval Request Comment
[Feature/Bug causing the regression]: 1323581
[User impact if declined]: None
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: Both commits in this bug require uplift.
[Is the change risky?]: No
[Why is the change risky/not risky?]: These patches move building of certain compiled tests from one stage of the build to another to make parts of the build safe to run in parallel, there should be no functional impact to the builds or tests in question.
[String changes made/needed]: None

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8826364 [details]
Bug 1200311 - Build the gtest libxul during the compile tier instead of package-tests.

Fix for failing tests, let's uplift.

Comment 17

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8834176 [details]
Bug 1200311 - Move dependentlibs.list.gtest generation to GENERATED_FILES.

[Triage Comment]

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/123ae818f185
https://hg.mozilla.org/releases/mozilla-aurora/rev/bc3e9cf39c49