Closed Bug 1320690 Opened 8 years ago Closed 7 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: 7 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: