Closed Bug 1320991 Opened 3 years ago Closed 3 years ago

--with-system-{nss,nspr} fail to build with --enable-tests


(Firefox Build System :: General, defect)

48 Branch
Not set


(firefox50 wontfix, firefox51 wontfix, firefox52 fixed, firefox53 fixed)

Tracking Status
firefox50 --- wontfix
firefox51 --- wontfix
firefox52 --- fixed
firefox53 --- fixed


(Reporter: jbeich, Assigned: mshal)



(Keywords: regression)


(1 file)

$ echo ac_add_options --with-system-nspr >>.mozconfig
$ echo ac_add_options --with-system-nss >>.mozconfig
$ ./mach build
../../../config/nsinstall -R -m 644 '../../../dist/bin/' 'objdir/js/src/gdb'
gmake[5]: Leaving directory 'objdir/js/src/gdb'
gmake[5]: Entering directory 'objdir/modules/libmar/tests'
gmake[5]: *** No rule to make target '../../../dist/bin/', needed by '../../../_tests/xpcshell/modules/libmar/tests/unit/'.  Stop.
Does this not break Linux distro builds because they don't --enable-tests? JC, do you know?
Flags: needinfo?(jjones)
No idea. Maybe Jed knows?
Flags: needinfo?(jjones) → needinfo?(jld)
I don't, but could this be a regression from bug 1253775?
Flags: needinfo?(jld) → needinfo?(mshal)
I don't think this worked even before bug 1253775, but I believe the fix is to just use USE_LIBS instead of the mess of manually handling nss libs.
Flags: needinfo?(mshal)
Assignee: nobody → mshal
Comment on attachment 8820880 [details]
Bug 1320991 - Support --with-system-{nss,nspr} in modules/libmar;

Jan, can you confirm that this works for you?
Attachment #8820880 - Flags: feedback?(jbeich)
Comment on attachment 8820880 [details]
Bug 1320991 - Support --with-system-{nss,nspr} in modules/libmar;

$ ./mach build
$ ./mach test modules/libmar/tests
 0:00.28 LOG: MainThread INFO Using at most 32 threads.
 0:00.28 SUITE_START: MainThread 3
 0:00.28 TEST_START: Thread-3 modules/libmar/tests/unit/test_sign_verify.js
 0:00.28 TEST_START: Thread-2 modules/libmar/tests/unit/test_extract.js
 0:00.29 TEST_START: Thread-1 modules/libmar/tests/unit/test_create.js
 0:00.42 TEST_END: Thread-2 PASS
 0:00.43 TEST_END: Thread-1 PASS
 0:00.76 TEST_END: Thread-3 PASS
 0:00.79 LOG: MainThread INFO INFO | Result summary:
 0:00.79 LOG: MainThread INFO INFO | Passed: 3
 0:00.79 LOG: MainThread INFO INFO | Failed: 0
 0:00.79 LOG: MainThread INFO INFO | Todo: 0
 0:00.79 LOG: MainThread INFO INFO | Retried: 0
 0:00.79 SUITE_END: MainThread

Ran 3 tests
Expected results: 3
Unexpected results: 0


However, you can completely remove[1] the section as modules/libmar/tests/ doesn't build any binaries worth having USE_LIBS for. "signmar" is built by modules/libmar/tool/ instead.

Attachment #8820880 - Flags: feedback?(jbeich) → feedback+
Comment on attachment 8820880 [details]
Bug 1320991 - Support --with-system-{nss,nspr} in modules/libmar;

Hmm, good point. I'll update the patch.
Attachment #8820880 - Flags: review?(mh+mozilla)
Comment on attachment 8820880 [details]
Bug 1320991 - Support --with-system-{nss,nspr} in modules/libmar;

This is probably fine, but it does something that, while I can see how it's related to the description, is completely different and may have implications that I don't know whether they have been assessed because there was no comment about it, either in the commit message or the bug.

That is, what this being removed here is copying nspr and nss to the test package for xpcshell libmar tests. I have no idea whether this breaks those tests, and whether this has been tested not to be breaking those tests.
Attachment #8820880 - Flags: review?(mh+mozilla) → review-

I've also updated the commit message with some more information. Let me know if you think of other possible implications that should be tested beforehand.
Per today's triage meeting, updating flags to fix-optional meaning we want a fix but we don't need to track this weekly.
Comment on attachment 8820880 [details]
Bug 1320991 - Support --with-system-{nss,nspr} in modules/libmar;
Attachment #8820880 - Flags: review?(mh+mozilla) → review+
Pushed by
Support --with-system-{nss,nspr} in modules/libmar; r=glandium
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Probably not worth uplifting to 51 at this point since we're about to spin the release candidate build, but it sounds like something we should uplift to Aurora in support of the next ESR.
Flags: needinfo?(mshal)
Version: Trunk → 48 Branch
Do people build ESR versions with custom mozconfigs? It should be harmless to uplift either way, but I don't know if it would be useful.
Flags: needinfo?(mshal)
(In reply to Michael Shal [:mshal] from comment #18)
> Do people build ESR versions with custom mozconfigs?

--with-system-nss --with-system-nspr are popular downstream but often together with --disable-tests. And tests can be used to debug downstream builds, especially if they have many patches applied.
Comment on attachment 8820880 [details]
Bug 1320991 - Support --with-system-{nss,nspr} in modules/libmar;

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]: Building Aurora (and eventually ESR, see #c17), will fail for developers who build with these custom mozconfig flags.
[Is this code covered by automated tests?]: Our build automation does not build with this particular set of flags.
[Has the fix been verified in Nightly?]:
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: It should not be
[Why is the change risky/not risky?]: The build config change is related only to test binaries produced by the build.
[String changes made/needed]:
Attachment #8820880 - Flags: approval-mozilla-aurora?
Comment on attachment 8820880 [details]
Bug 1320991 - Support --with-system-{nss,nspr} in modules/libmar;

make tests work with system-{nss,nspr}, aurora52+
Attachment #8820880 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.