1520646 - NSPR should use UNIFIED_SOURCES

Status: RESOLVED FIXED

(NSPR :: NSPR, enhancement)

Description

[(Away)]

In a build log I happened to notice a large number of non-unified files coming from https://searchfox.org/mozilla-central/source/config/external/nspr/pr/moz.build

Comment 1

[(Away)]

Attachment: Initial patch, need to split out moz.build versus NSPR-repo changes

Comment 2

[(Away)]

Attachment: NSPR repo fixes

Two small fixes for issues that only showed up in a unified build:

• _MD_win32_map_connect_error in reality returns void

• _pr_test_ipv6_socket is a PR_IMPLEMENT, so the declaration needs PR_EXTERN.

Comment 3

[(Away)]

Attachment: moz.build changes

I'm quite pleased that only two files needed hand-holding. \o/

Comment 4

[Kai Engert (:KaiE:)]

David, could you please add a reference that explains the meaning of unified sources?

Comment 5

[(Away)]

(In reply to Kai Engert (:kaie:) from comment #4)

David, could you please add a reference that explains the meaning of unified sources?

Sure, here's one: https://lists.mozilla.org/pipermail/dev-platform/2013-November/001998.html

Some projects also refer to them as unity or jumbo builds: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/ThDAjO7fTro/nXX8kJFBAgAJ

Comment 6

[Kai Engert (:KaiE:)]

https://hg.mozilla.org/projects/nspr/rev/494198b1bad6

Comment 7

[Kai Engert (:KaiE:)]

I can uplift an NSPR 4.21 snapshot into mozilla-inbound soon, and can include the moz.build changes at the same time, if you want.

Comment 8

[(Away)]

(In reply to Kai Engert (:kaie:) from comment #7)

I can uplift an NSPR 4.21 snapshot into mozilla-inbound soon, and can
include the moz.build changes at the same time, if you want.

Works for me, thanks!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b4cd1f37414
Use UNIFIED_SOURCES in NSPR, r=glandium

Comment 10

[Noemi Erli[:noemi_erli]]

Backed out 2 changesets (Bug 1526010, Bug 1520646) mass mochitest failures UPGRADE_NSPR_RELEASE CLOSED TREE

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=5b4cd1f3741439edc002b54780c61b66f38427e5&selectedJob=227179183

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=227179183&repo=mozilla-inbound&lineNumber=9761
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=227179103&repo=mozilla-inbound&lineNumber=4887

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/42763a2bd75901be5380580a9817c06a4c9a8a2a

Comment 11

[(Away)]

Oh, oops, I accidentally typed this on the other bug in the push. Pasting it here for completeness:

Kai, unless you can see some other reason for the failure within the NSPR code changes, then I think it's likely that the unified build patch revealed some additional subtle issues that only came up at runtime. I'll look more into it...

Comment 12

[Kai Engert (:KaiE:)]

David, it should be fine to have the "NSPR repo fixes" changes in the mozilla tree, while not yet having the "moz.build changes", right?

(If yes, I can uplift the NSPR beta snapshot again, and let you handle the build changes on your own, separately. You might want to use a separate bug in the firefox build system component for that.)

Comment 13

[(Away)]

(In reply to Kai Engert (:kaie:) from comment #12)

David, it should be fine to have the "NSPR repo fixes" changes in the
mozilla tree, while not yet having the "moz.build changes", right?

(If yes, I can uplift the NSPR beta snapshot again, and let you handle the
build changes on your own, separately. You might want to use a separate bug
in the firefox build system component for that.)

To answer your question very narrowly: yes, it should be safe to land the first patch containing the NSPR repo fixes by itself.

But: is it ok if I take some time to diagnose the failures first? If the investigation turns up more issues that need to be fixed on the NSPR side, I would be sad to have to wait potentially another 5 months for the next merge.

Comment 14

[Kai Engert (:KaiE:)]

David, no need to worry, I don't want to declare a release yet! We have rougly 4 weeks until the end of the current 6 weeks cycle, I'd like to release NSPR 4.21 at the end of the current dev cycle, maybe around March 4th.

The reason I'd like to uplift the current NSPR snapshot into m-i is for testing it in nightly FF builds, but it's fine to make more changes.

Comment 15

[(Away)]

The cause of the test failures was w32poll.c, which wanted a custom value of FD_SETSIZE used during its include of winsock.h

But in the process of investigating this, I set FILES_PER_UNIFIED_FILE = 999 to tease out other latent issues by maximizing the amount of crowdedness/interference in the unified translation units, and I'll add the additional fixes for those issues here too.

Comment 16

[(Away)]

Attachment: More NSPR repo fixes

Updates some more declarations to match their definitions.

Comment 17

[(Away)]

Attachment: moz.build changes

Here's the updated moz.build, though I'm not quite sure what to do about this process-wise. I guess once we have the NSPR bits in place I can do this in a separate build system bug like you said.

Comment 18

[Kai Engert (:KaiE:)]

Comment on attachment 9043476 [details] [diff] [review]
More NSPR repo fixes

https://hg.mozilla.org/projects/nspr/rev/40ce5d162994

Comment 19

[(Away)]

Kai, I have one more issue that I was getting ready to work around on the moz.build side, but actually now I'm wondering if it is a code problem.

unix.c defines its own _pr_rename_lock in addition to the ptio.c one:
https://searchfox.org/mozilla-central/search?q=_pr_rename_lock&redirect=false

But nothing in unix.c refers to it meaningfully (just Init and Cleanup). Was there intended to be some use of this variable (in which case I'll de-unify that file so the two variables don't interfere with each other) or should the unix one simply be removed?

Comment 20

[Kai Engert (:KaiE:)]

Let's reopen if this isn't done.

unix.c defines its own _pr_rename_lock in addition to the ptio.c one:
https://searchfox.org/mozilla-central/search?q=_pr_rename_lock&redirect=false

But nothing in unix.c refers to it meaningfully (just Init and Cleanup).

I see additional uses _MD_rename, _MD_mkdir, _MD_Open

Both files the variable is declared "static", that means it isn't visible outside the scope of the file.
Why do you consider that variable problem?

Was there intended to be some use of this variable

I don't know the history. But it seems it's indeed being used, or do I miss anything?

(in which case I'll de-unify

do you mean, rename it? Why would it be necessary, it the variables are static?

that file so the two variables don't interfere with each other)

Because both are defined static, I'd assume they don't interfere?

Comment 21

[(Away)]

(In reply to Kai Engert (:kaie:) from comment #20)

I see additional uses _MD_rename, _MD_mkdir, _MD_Open

Oh, sorry, I didn't see those in the "uses" section.

Because both are defined static, I'd assume they don't interfere?

During the unification process, the two files are concatenated into a single .c file, and (TIL) the C language will silently allow the redefinition, so they become the same variable. Scary.

So I'll need to remove one of those two files from the unified set.

Comment 22

[Kai Engert (:KaiE:)]

I suggest to simply rename the variable in one of the files.

Comment 23

[(Away)]

Attachment: Rename unix.c's _pr_rename_lock to avoid unified-build conflict with ptio.c.

Comment 24

[Kai Engert (:KaiE:)]

Comment on attachment 9045489 [details] [diff] [review]
Rename unix.c's _pr_rename_lock to avoid unified-build conflict with ptio.c.

https://hg.mozilla.org/projects/nspr/rev/202055079f59

Comment 25

[(Away)]

Thanks Kai. Knock on wood, that should be all the NSPR changes that we need. I'll land the moz.build separately as you suggested.