1399095 - Allow nss-try to be used to test NSPR changes

Status: RESOLVED FIXED

(NSS :: Test, enhancement, P3)

Description

[Kai Engert (:KaiE:)]

We don't have separate NSPR infrastructure for continuous integration.

It would be good to be able to test experimental NSPR code changes as part of the NSS infrastructure.

I suggest that the NSS taskcluster script is extended in the following way:

Between retrieving the NSPR code and starting the NSS/NSPR build, insert the following step:

- check if a special file is contained inside the NSS directory,
  for example
    nss/nspr.patch

- if the file is found, apply the patch on top of nspr

This should be sufficient.

IIUC, the script automation/taskcluster/scripts/build.sh
is the main driver and is the script that would have to be modified.

Comment 1

[Kai Engert (:KaiE:)]

Attachment: 1399095.patch

Should this work?

I think using -p1 should be compatible with most scenarios, it's compatible with the output produced "hg diff".

Comment 2

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

Bulk change per wleung.

Comment 3

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8907039 [details] [diff] [review]
1399095.patch

Review of attachment 8907039 [details] [diff] [review]:
-----------------------------------------------------------------

This will only change the gyp builds. You'd have to do the same for them (https://searchfox.org/nss/rev/670616c29ffdfe1ae74455173a835084b6e1fb91/automation/taskcluster/scripts/build.sh).

Then I'm wondering if this is the right place to do it in general. I think I'd prefer when we'd add a try syntax (maybe something like -p nspr) that uses a separate script to apply patches.

Comment 4

[Kai Engert (:KaiE:)]

(In reply to Franziskus Kiefer [:franziskus] (Away until October 2019) from comment #3)

This will only change the gyp builds. You'd have to do the same for them
(https://searchfox.org/nss/rev/670616c29ffdfe1ae74455173a835084b6e1fb91/
automation/taskcluster/scripts/build.sh).

I'm attaching a more complete patch v2. Using it, I was able to test bug 1535665 and bug 1375876.

Then I'm wondering if this is the right place to do it in general. I think
I'd prefer when we'd add a try syntax (maybe something like -p nspr) that
uses a separate script to apply patches.

Maybe it's not the ideal place, but IMHO it's a reasonable compromise.

• We don't need nspr try patches often, I didn't have a need during the past 2 years apparently. This might not justify the extra work for a more advanced solution. And the "right" approach would be to have NSPR as its own taskcluster environment, which seems overkill to do.
• IMHO we don't need a separate script to apply NSPR patches. Preparing one combined patch for a try run is simple to do.

Comment 5

[Kai Engert (:KaiE:)]

Attachment: 1399095-v2.patch

Comment 6

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 9074733 [details] [diff] [review]
1399095-v2.patch

Hmm... is there something we can do to help avoid accidentally merging an nspr.patch into the real tree?

I'd almost rather have this check for a file that indicates an NSPR version checkout, as that seems slightly less fraught-with-peril.

Comment 7

[Kai Engert (:KaiE:)]

(In reply to J.C. Jones [:jcj] (he/him) from comment #6)

I'd almost rather have this check for a file that indicates an NSPR version
checkout, as that seems slightly less fraught-with-peril.

Your suggestion requires that we commit a change to NSPR, before we can test it in try, right?

I'd like to be able to execute a try build, without having to check in to NSPR.

Hmm... is there something we can do to help avoid accidentally merging an
nspr.patch into the real tree?

I don't see how.

I wish I had this available now, because I'd like to work on bug 1385039, but it's difficult to test that without being able to run try builds. I need NSPR changes for that to work (makefile targets), but I'd like to avoid checking in experimental code to NSPR.

Comment 8

[Kai Engert (:KaiE:)]

Well, I guess I could include this whole patch each time I want to run an NSPR try run... But that's overhead, and the patch will require constant merging. (It already bitrotted.)

Comment 9

[Kai Engert (:KaiE:)]

We could use a really ugly filename, like nspr-experimental-patch-for-try-builds-dont-ever-check-this-in.patch

Comment 10

[Kai Engert (:KaiE:)]

Attachment: 1399095-v3.patch

merged

Comment 11

[J.C. Jones [:jcj] (he/they)]

Kevin, this patch looks good to me, but I wonder: Do you think we should also check an env var to see whether we're on NSS or NSS-try and make it so we fail on NSS if there's a patch checked in?

Like, that's the wrong time to fail, frankly -- it's too late. But maybe that's better than nothing?

What I'd really like is to do a pre-receive hook in hg.mozilla.org/projects/nss that aborts pushes with this patch in it, I think. I wonder how we'd get such a thing.

Thoughts?

Comment 12

[Kai Engert (:KaiE:)]

Maybe a hook would make things complicated for nss-try, too?
A hook in the NSS repository wouldn't protect mozilla-central from accidentally getting a nspr.patch.

Here are two alternative ideas (in addition to the earlier "ugly filename" idea):

We could ignore the nspr.patch file if MOZILLA_CLIENT is defined.
NSS doesn't set MOZILLA_CLIENT. It's defined if NSS is built as part of a Mozilla application like Firefox.

Or we could ignore nspr.patch if lib/nss/nss.h defines NSS_BETA as PR_TRUE.

Comment 13

[Kevin Jacobs [:kjacobs]]

Minimally, I'd like us to ignore nspr.patch if MOZILLA_CLIENT, and fail if NSS_HEAD_REPOSITORY is defined as anything except try (so that NSS taskcluster would catch it).

But I greatly prefer having this behind try syntax, since the patch would simply be ignored without an explicit build option. I don't think we'd need any more scripts, just a new switch in build.sh.

Comment 14

[Kai Engert (:KaiE:)]

Attachment: 1399095-v5.patch

This patch v5 will only apply nspr.patch if environment variable ALLOW_NSPR_PATCH=1 is set.
The variable can be set using nss-try syntax, by adding the --nspr-patch flag.

The following try build demonstrates that this change covers all platforms and build types. I used an invalid nspr.patch file. Because all builds fail, it shows that each build attempted to use the broken patch file:
https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=02fc866e05971ae04ebf800f1aa359a15a0fd5d5

The following try build demonstrates that the nspr.patch is ignored if the --nspr-patch flag is omitted:
https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=e107b2d29243b7cac2ed7e3ef21f7da72daa3f54

My current motivation to work on this nss-try fix for NSPR is bug 1581890.

The following try build uses a very large nspr.patch (from bug 1581890), and by looking at the logs, we see that the patch file is successfully applied:
https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=8fb13f219d4d4c0013058dccfdff50084daa1cff

Comment 15

[Kevin Jacobs [:kjacobs]]

Comment on attachment 9093595 [details] [diff] [review]
1399095-v5.patch

Review of attachment 9093595 [details] [diff] [review]:
-----------------------------------------------------------------

I think this is a sufficient safeguard, just two nits:

1) The "--nspr-patch" `includes` should be called on `match[1]` rather than `comment` 
2) Compound conditionals would be a little nicer than nested `if`'s.

r+ modulo item 1. Thanks!

Comment 16

[Kai Engert (:KaiE:)]

Thanks, requested changes applied and pushed:
https://hg.mozilla.org/projects/nss/rev/6e1a8a7cb4694c809a32061f30124392a24f7cb5