NSPR should use UNIFIED_SOURCES

RESOLVED FIXED in 4.21

Status

enhancement
RESOLVED FIXED
4 months ago
3 months ago

People

(Reporter: dmajor, Assigned: dmajor)

Tracking

other
4.21
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Assignee)

Description

4 months ago

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

(Assignee)

Comment 2

4 months ago

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.

Attachment #9037733 - Attachment is obsolete: true
Attachment #9038566 - Flags: review?(kaie)
(Assignee)

Comment 3

4 months ago
Posted patch moz.build changes (obsolete) — Splinter Review

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

Attachment #9038567 - Flags: review?(mh+mozilla)
Attachment #9038567 - Flags: review?(mh+mozilla) → review+

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

(Assignee)

Comment 5

4 months ago

(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

Updated

3 months ago
Attachment #9038566 - Flags: review?(kaie) → review+
Status: NEW → RESOLVED
Last Resolved: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → 4.21

Updated

3 months ago
Blocks: 1526010

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.

(Assignee)

Comment 8

3 months ago

(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

3 months ago
Pushed by kaie@kuix.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b4cd1f37414
Use UNIFIED_SOURCES in NSPR, r=glandium

Updated

3 months ago
Flags: needinfo?(kaie+oldbugzilla) → needinfo?(kaie)

Updated

3 months ago
Flags: needinfo?(kaie)
(Assignee)

Comment 11

3 months ago

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...

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.)

Updated

3 months ago
Flags: needinfo?(dmajor)
(Assignee)

Comment 13

3 months ago

(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.

Flags: needinfo?(dmajor)

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.

(Assignee)

Comment 15

3 months ago

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.

(Assignee)

Comment 16

3 months ago

Updates some more declarations to match their definitions.

Attachment #9043476 - Flags: review?(kaie)
(Assignee)

Comment 17

3 months ago

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.

Attachment #9038567 - Attachment is obsolete: true

Updated

3 months ago
Attachment #9043476 - Flags: review?(kaie) → review+

Updated

3 months ago
Attachment #9038566 - Flags: checked-in+
(Assignee)

Comment 19

3 months ago

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?

Flags: needinfo?(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?

Status: RESOLVED → REOPENED
Flags: needinfo?(kaie)
Resolution: FIXED → ---
(Assignee)

Comment 21

3 months ago

(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.

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

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
Attachment #9045489 - Flags: review?(kaie)
Attachment #9045489 - Flags: review+
Attachment #9045489 - Flags: checked-in+
(Assignee)

Updated

3 months ago
Blocks: 1530483
(Assignee)

Comment 25

3 months ago

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.

Status: REOPENED → RESOLVED
Last Resolved: 3 months ago3 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.