1227248 - Don't spend minutes generating .pem certificates for --disable-compile-environment builds

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

Bug 1181823 moved a bunch of .pem certificate generation to build time.  The genearation is really time consuming; in fact, I think it's the second-longest part of a mobile/android --disable-compile-environment build.

This ticket tracks not generating those certificates in this mode, or finding a way to generate these only when they're needed (package-tests?).  Based on the discussion in https://bugzilla.mozilla.org/show_bug.cgi?id=1181823#c0 ("hash of the root test certificate is hard-coded into the browser in ExtendedValidation.cpp"), sounds like just-in-time generation is hard.  But perhaps we could generate just the root test certificate at build time, and generate the rest at package-tests time?

In general, disabling this based on --disable-compile-environment is not desirable.  I have patches to add --enable-artifact-builds, which would be a better statement of intention for this change.

Comment 1

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

(In reply to Nick Alexander :nalexander from comment #0)
> Bug 1181823 moved a bunch of .pem certificate generation to build time.  The
> genearation is really time consuming; in fact, I think it's the
> second-longest part of a mobile/android --disable-compile-environment build.
> 
> This ticket tracks not generating those certificates in this mode, or
> finding a way to generate these only when they're needed (package-tests?). 
> Based on the discussion in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1181823#c0 ("hash of the root
> test certificate is hard-coded into the browser in ExtendedValidation.cpp"),
> sounds like just-in-time generation is hard.  But perhaps we could generate
> just the root test certificate at build time, and generate the rest at
> package-tests time?

dkeeler: could you comment on which test certificates are really needed at Firefox build time?

gps: you've done a bunch of work improving package-tests.  Is this feasible?

Comment 2

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

> In general, disabling this based on --disable-compile-environment is not
> desirable.  I have patches to add --enable-artifact-builds, which would be a
> better statement of intention for this change.

These patches are attached to Bug 1216817.

Comment 3

[Dana Keeler (she/her) (use needinfo) [:keeler]]

The generation of the EV test certificates is deterministic, so there actually shouldn't be a built-time dependency on the test certificates. That is, if you don't want to run any tests, I don't think any of the test certificates are required.

Comment 4

[Gregory Szorc [:gps]]

Yes, it is feasible to move certificate generation to a packaging time action. However, packaging is independent of running the tests and we still need a "hook" in "run tests" to build them if necessary. Where that hook lives probably depends on how developers run these tests. What's the typical developer workflow? I'm thinking we could hack them as e.g. a "make check" target along with MOZ_AUTOMATION or configure changes to influence default behavior.

Comment 5

[Mike Hommey [:glandium]]

It's a hard problem. But there's a simpler realization: you can't run xpcshell tests if you build with --disable-compile-environment because you don't have a xpcshell to run them... so these certificates are effectively useless.

Comment 6

[Mike Hommey [:glandium]]

(and artifact builds don't come with a xpcshell binary either)

Comment 7

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a7b1cf36f45

Comment 8

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

Attachment: MozReview Request: Bug 1227248 - Part 1: Allow extending StrictOrderingOnAppendListWithFlags. r?gps

Bug 1227248 - Part 1: Allow extending StrictOrderingOnAppendListWithFlags. r?gps

In order to use StrictOrderingOnAppendListWithFlags instances in
mozbuild template functions, we need += to work correctly.  This patch
implements extend and the associated functions (including +=),
disallowing some behaviour where convenient.

There's a subtle point hidden in the isinstance() tests: before this
patch, it was not easy to compare two
StrictOrderingOnAppendListWithFlags instances to see if they had the
*same* set of flags.  That was because two instances may not have the
same class, and would only share the common
StrictOrderingOnAppendList, which isn't enough to infer the presence
of flags.  To be slightly more clear, concrete instances will have
class StrictOrderingOnAppendListWithFlagsSpecialization (although
there are still multiple instances of that class) and all extend from
the unique class StrictOrderingOnAppendListWithFlags.

Comment 9

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

Attachment: MozReview Request: Bug 1227248 - Part 2: Add GeneratedTest{Certificate,Key} mozbuild templates. r?gps

Bug 1227248 - Part 2: Add GeneratedTest{Certificate,Key} mozbuild templates. r?gps

Comment 10

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

Attachment: MozReview Request: Bug 1227248 - Part 3: Make GeneratedTest{Certificate,Key} no-op when --disable-compile-environment. r?gps

Bug 1227248 - Part 3: Make GeneratedTest{Certificate,Key} no-op when --disable-compile-environment. r?gps

This implements glandium's suggestion from
https://bugzilla.mozilla.org/show_bug.cgi?id=1227248#c5: since it's
not easy to run xpcshell tests in --disable-compile-environment builds
(and, right now, in artifact builds), let's just skip this work
entirely in those situations.  This saves about 30s of build time on
my machine.

Comment 11

[Mike Hommey [:glandium]]

https://reviewboard.mozilla.org/r/27839/#review25005

::: build/test_templates.mozbuild:17
(Diff revision 1)
> +    for part in RELATIVEDIR.split('/'): # TODO: Handle OS-specific path seperater.

paths in moz.build are always using '/', so there is nothing to note as a TODO.

Comment 12

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/27837/#review25127

::: python/mozbuild/mozbuild/test/test_util.py:481
(Diff revision 1)
> +            self.assertEqual(len(l), 6)
> +            self.assertEqual(l['a'].foo, True)
> +            self.assertEqual(l['b'].bar, 'bar')
> +            self.assertEqual(l['d'].foo, True)
> +            self.assertEqual(l['e'].bar, 'bar')

You want to assert that c and f are in the list too.

::: python/mozbuild/mozbuild/util.py:497
(Diff revision 1)
> +                # The fact that other's flags are preserved (rather than self's)
> +                # is an implementation detail.
> +                result._update_flags(self)

The comment says other's flags are preserved but we're passing `self` to `_update_flags`. This feels wrong and doesn't agree with the use of `other` below. Although, since we disallow updates when both values have the same flags, I guess it doesn't matter, right?

Comment 13

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/27839/#review25129

Very nice cleanup.

Comment 14

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/27841/#review25131

Comment 15

[Nick Alexander :nalexander [he/him] (PTO Feb 24-March 3)]

https://hg.mozilla.org/integration/fx-team/rev/8ce39c03f90b01f476b890904090b58d7f09300e
Bug 1227248 - Part 1: Allow extending StrictOrderingOnAppendListWithFlags. r=gps

https://hg.mozilla.org/integration/fx-team/rev/009b3d8932c655b68198189610d545f7f39a8ee3
Bug 1227248 - Part 2: Add GeneratedTest{Certificate,Key} mozbuild templates. r=gps

https://hg.mozilla.org/integration/fx-team/rev/634a4690d5a09609ea13f28aacde14c1b4079a9b
Bug 1227248 - Part 3: Make GeneratedTest{Certificate,Key} no-op when --disable-compile-environment. r=gps

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8ce39c03f90b
https://hg.mozilla.org/mozilla-central/rev/009b3d8932c6
https://hg.mozilla.org/mozilla-central/rev/634a4690d5a0