Closed Bug 1227248 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox46 fixed)

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: nalexander, Unassigned)

References

Details

Attachments

(3 files)

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.
(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?
Flags: needinfo?(gps)
Flags: needinfo?(dkeeler)
> 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.
See Also: → 1216817
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.
Flags: needinfo?(dkeeler)
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.
Flags: needinfo?(gps)
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.
(and artifact builds don't come with a xpcshell binary either)
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.
Attachment #8698139 - Flags: review?(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.
Attachment #8698141 - Flags: review?(gps)
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.
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?
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
Attachment #8698139 - Flags: review?(gps) → review+
Attachment #8698140 - Flags: review?(gps) → review+
Attachment #8698141 - Flags: review?(gps) → review+
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.