Closed Bug 1320690 Opened 9 years ago Closed 9 years ago

Hook bundled NSS build on BSDs again

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Platform:
Unspecified
FreeBSD
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox53 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: jbeich, Unassigned)

References

Details

Attachments

(1 file)

Bug 1320690 - Re-enable bundled NSS on BSDs.
58 bytes, text/x-review-board-request
ted
: review+
Details
This is mozilla-central glue for bug 1311619.
OS: Unspecified → FreeBSD
Comment on attachment 8814916 [details] Bug 1320690 - Re-enable bundled NSS on BSDs. https://reviewboard.mozilla.org/r/95996/#review96126 ::: security/generate_mapfile.py:22 (Diff revision 1) > > > def main(output, input): > # There's a check in old-configure.in under the system-nss handling > # that should match this. > - if buildconfig.substs['OS_ARCH'] not in ('Linux', 'Darwin'): > + if buildconfig.substs['OS_ARCH'] in ('Windows'): This does not do what you're thinking of. Either use `in ('Windows',)`, with the comma, or `== 'Windows'`. ::: security/generate_mapfile.py:27 (Diff revision 1) > - if buildconfig.substs['OS_ARCH'] not in ('Linux', 'Darwin'): > + if buildconfig.substs['OS_ARCH'] in ('Windows'): > print "Error: unhandled OS_ARCH %s" % buildconfig.substs['OS_ARCH'] > return 1 > - is_linux = buildconfig.substs['OS_ARCH'] == 'Linux' > + is_linux = buildconfig.substs['OS_ARCH'] != 'Darwin' > > with open(input, 'rb') as f: Seems like a bit of a misnomer now.
Attachment #8814916 - Flags: review-
Comment on attachment 8814916 [details] Bug 1320690 - Re-enable bundled NSS on BSDs. https://reviewboard.mozilla.org/r/95996/#review96128 I think this needs some tweaks in generate_mapfile.py, but otherwise should be fine. I'm sorry we didn't get this sorted out before I landed the build changes. ::: security/generate_mapfile.py:22 (Diff revision 1) > > > def main(output, input): > # There's a check in old-configure.in under the system-nss handling > # that should match this. > - if buildconfig.substs['OS_ARCH'] not in ('Linux', 'Darwin'): > + if buildconfig.substs['OS_ARCH'] in ('Windows'): I'm not sure this is terribly useful like this. Maybe just remove this entirely? Alternately, you could change this to feature-test the actual thing we care about: GNU LD or Apple LD. We do have both `GNU_LD` and `GCC_USE_GNU_LD` available, so maybe the right check here is: `if not buildconfig.substs['GNU_LD'] and buildconfig.substs['OS_ARCH'] != 'Darwin':` because this script is only capable of writing out GNU binutils-compatible linker scripts, and Apple LD `-exported_symbols_list` files. ::: security/generate_mapfile.py:25 (Diff revision 1) > # There's a check in old-configure.in under the system-nss handling > # that should match this. > - if buildconfig.substs['OS_ARCH'] not in ('Linux', 'Darwin'): > + if buildconfig.substs['OS_ARCH'] in ('Windows'): > print "Error: unhandled OS_ARCH %s" % buildconfig.substs['OS_ARCH'] > return 1 > - is_linux = buildconfig.substs['OS_ARCH'] == 'Linux' > + is_linux = buildconfig.substs['OS_ARCH'] != 'Darwin' That doesn't seem right. Presumably the distinction here is "Apple's linker" vs. "GNU binutils linker". Maybe change this to `is_darwin = buildconfig.substs['OS_ARCH'] == 'Darwin'` and then change the rest of the script to match? For the comments you'd do `s/non-Linux/Darwin/`.
Attachment #8814916 - Flags: review?(ted) → review-
Oops, I haven't addressed Ted's review yet.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a64a70febadd9fc507f7ab2de18e0be5e66fba3a (In reply to Ted Mielczarek [:ted.mielczarek] from comment #4) > Maybe just remove this entirely? Done. Less code to maintain.
Comment on attachment 8814916 [details] Bug 1320690 - Re-enable bundled NSS on BSDs. https://reviewboard.mozilla.org/r/95996/#review97516 ::: old-configure.in (Diff revision 3) > - # This is to match the conditions in security/generate_mapfile.py, > - # plus Windows which doesn't run that script. > - WINNT|Darwin|Linux) > - ;; > - *) > - AC_MSG_ERROR([building in-tree NSS is not supported on this platform. Use --with-system-nss]) Feels like we should still have *something* here, but I don't know precisely what it would be, so I guess we can just let it slide. I'm sure this isn't the only thing that will break when porting to a new platform. ::: security/generate_mapfile.py:45 (Diff revision 3) > if i != -1: > - if is_linux: > + if not is_darwin: > line = line[:i+1] > else: > line = line[:i] > # On non-Linux, symbols get an underscore in front. nit: change the comment here to match the conditional.
Attachment #8814916 - Flags: review?(ted) → review+
Comment on attachment 8814916 [details] Bug 1320690 - Re-enable bundled NSS on BSDs. https://reviewboard.mozilla.org/r/95996/#review97516 > Feels like we should still have *something* here, but I don't know precisely what it would be, so I guess we can just let it slide. I'm sure this isn't the only thing that will break when porting to a new platform. OK. Solaris, GNU/kFreeBSD, Darwin/X11, Windows/MINGW are stil broken with bundled NSS. Those are Tier3, the rest is beyond hope for mozilla-central to even care.
Comment on attachment 8814916 [details] Bug 1320690 - Re-enable bundled NSS on BSDs. Not sure how to reset r+ via MozReview
Attachment #8814916 - Flags: review+ → review?(ted)
Comment on attachment 8814916 [details] Bug 1320690 - Re-enable bundled NSS on BSDs. https://reviewboard.mozilla.org/r/95996/#review99436 This looks good, thanks.
Attachment #8814916 - Flags: review?(ted) → review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f5fee03d16ab Re-enable bundled NSS on BSDs. r=ted
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f5fee03d16ab
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: