All users were logged out of Bugzilla on October 13th, 2018

xpcshell test certificates are always rebuilt and take about half the time of a no-op build

RESOLVED FIXED in Firefox 42

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: glandium, Assigned: keeler)

Tracking

unspecified
mozilla42
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
STR:
- mach build
- mach build

On my machine the second "mach build", which is (mostly) a no-op takes 45s, 23s of which are spent rebuilding all the xpcshell test certificates.

I have a simple patch that removes the buildid from the script dependencies, making the build skipped, but we have to keep in mind bug 1170431. 
That patch however hardcodes the date, which will likely fail after a while.

That being said, maybe we should step back a little and think about it from a different angle: if those certificates have emission dates and expiry dates set in stone in the -test.zip file, downloading that file in a few months to re-run the xpcshell tests without rebuilding from the given changeset will likely have an expiry problem. Which makes me wonder if those certificates shouldn't, in fact, be created during the test run, instead of during the build.

With that being said, I can understand that this would be much more work than one may want to put (at least speaking for myself), and at the moment I care more about making no-op builds not waste their time with this.

A kind of reasonable option, I guess, would be to move the creation of those certificates to a separate "tier" that only gets built when we build the test archive or before running xpcshell tests.

Alternatively, we could have a buildid-like file that the build system only changes, say, once a week (duration depending on what the expiry for those is, and what the expectations for those certificates are)

Thoughts?
(In reply to Mike Hommey [:glandium] from comment #0)
> Which makes me wonder if those
> certificates shouldn't, in fact, be created during the test run, instead of
> during the build.
<snip/>
> A kind of reasonable option, I guess, would be to move the creation of those
> certificates to a separate "tier" that only gets built when we build the
> test archive or before running xpcshell tests.

Something like this sounds like the right answer to me.

I have pretty much zero knowledge of how the build system works; are you able to explain at a high level what we'd need to do to achieve this?
 
> Alternatively, we could have a buildid-like file that the build system only
> changes, say, once a week (duration depending on what the expiry for those
> is, and what the expectations for those certificates are)

This could also work.
For the certs we use for Mochitest, we check them into the tree and give them a 10 year expiry. This sucks because it's a potential future failure, but it's better than regenerating them all the time.

Could we do something hacky like set the mtime of the generated certs to their expiry date, and then make them have a dependency on a file that we `touch` so they'd only get regenerated if they were expired? (Maybe we'd want to make it expiry - some time so that they would get regenerated before you wound up using them.)
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #2)
> For the certs we use for Mochitest, we check them into the tree and give
> them a 10 year expiry. This sucks because it's a potential future failure,
> but it's better than regenerating them all the time.

As Bug 1174288 indicates, there's an active effort to move toward more frequent certificate generation for xpcshell tests. I'm guessing that the main motivation here is ensuring out tests are less fragile (since we avoid temptation to hard-code certificate info in tests, etc); of course, Keeler will know for sure.
There are a few motivations behind generating these certificates programmatically rather than checking them into the tree. See bug 1166976 comment 0.

Generating them when the tests run rather than at compile time might be a reasonable solution, but I'm concerned about tests timing out on the B2G emulators. Having another tier-like thing also seems like a good approach if it's not a lot of work.

We could also do something like take the major version of gecko, guess when it'll be released based on the 6-week cycle, and have the certificates be valid for a year or so before and after that. That way, they wouldn't depend on the buildid but would still have a reasonable validity period. I'm worried that will be a bit fragile, though, because the schedule isn't always 6 weeks.
(Reporter)

Updated

3 years ago
Depends on: 1181450
Created attachment 8634462 [details]
MozReview Request: bug 1179660 - define 'now' as the first second of the current year for pycert r?Cykesiopka

bug 1179660 - define 'now' as the first second of the current year for pycert r?Cykesiopka

This is to avoid a dependency on the buildid so we don't have to
regenerate all of the test certificate with every ./mach build.
This can cause problems very near midnight on New Year's Eve.
If this happens, kick off a new build and consider reexamining
your current work-life balance.
Attachment #8634462 - Flags: review?(cykesiopka.bmo)
(In reply to David Keeler [:keeler] (use needinfo?) from comment #5)
> Created attachment 8634462 [details]
> MozReview Request: bug 1179660 - define 'now' as the first second of the
> current year for pycert r?Cykesiopka

This isn't a perfect solution, but it'll do for now. We can do something more comprehensive later, but for now we need to fix the immediate issue of doing a lot of work every single build for no good reason.
(Reporter)

Comment 7

3 years ago
I'm working on moving those tests out of the normal build, fwiw.

Comment 8

3 years ago
Comment on attachment 8634462 [details]
MozReview Request: bug 1179660 - define 'now' as the first second of the current year for pycert r?Cykesiopka

https://reviewboard.mozilla.org/r/13401/#review12047

LGTM.
Adding a time bomb is never fun, but this does workaround the issue, and this will probably get defused soon anyways.

::: security/manager/ssl/tests/unit/test_cert_eku/moz.build:569
(Diff revision 1)
> -    props.inputs = [input_file, '!/config/buildid']
> +    props.inputs = [input_file]

test_cert_eku/generate.py needs to be updated for this as well.
Attachment #8634462 - Flags: review?(cykesiopka.bmo) → review+
Is this patch ready to land?
Flags: needinfo?(dkeeler)
(In reply to David Major [:dmajor] from comment #9)
> Is this patch ready to land?

Depends on how close :glandium is to the real solution rather than this temporary fix.
Flags: needinfo?(dkeeler) → needinfo?(mh+mozilla)
I find it frustrating when improvements get put off in anticipation of other changes that may or may not ever materialize. In the meantime lots of developers are still losing lots of time on this.
(Reporter)

Comment 12

3 years ago
Please land.
Flags: needinfo?(mh+mozilla)
Keywords: checkin-needed
Duplicate of this bug: 1187154
https://hg.mozilla.org/mozilla-central/rev/eb0a49e8322a
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
It seems to me this bug still exists.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Comment 18

3 years ago
bug 1188468 is opened for this and has some debugging information.
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Depends on: 1188468
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.