370062 - nss changes for OpenBSD

Status: RESOLVED FIXED

(NSS :: Build, defect, P3)

Description

[Martynas Venckus]

User-Agent:       Mozilla/5.0 (X11; U; OpenBSD i386; en-US; rv:1.8.1.1) Gecko/20070207 Firefox/2.0.0.1
Build Identifier: 

security/coreconf/OpenBSD.mk should use arch(1) to obtain the actual CPU_ARCH. This fixes things like OpenBSD/zaurus.
The diff is available at http://www.altroot.org/OpenBSD_mk.diff

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[timeless]

Attachment: from comment 0

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 254723 [details] [diff] [review]
from comment 0

I have no way to test this patch.  I wonder if it works for all the relevant types of "arch", or if it only works for i386.  

I'd like you to ask some other members of the OpenBSD community to "weigh in" on this proposed change before approving it.

I would accept r+ from OpenBSD folks for this patch (only), in lieu of the usual NSS team r+.

Comment 3

[Martynas Venckus]

From the man page:

"The arch and machine commands display the machine's architecture in
slightly different ways.  arch by default displays the application archi-
tecture, defined by both the operating system and the instruction set ar-
chitecture"

http://www.openbsd.org/cgi-bin/man.cgi?query=arch&apropos=0&sektion=0&manpath=OpenBSD+Current&arch=i386&format=html

The patches from OpenBSD ports:

http://www.openbsd.org/cgi-bin/cvsweb/ports/www/mozilla-firefox/patches/patch-security_coreconf_OpenBSD_mk?rev=1.6
http://www.openbsd.org/cgi-bin/cvsweb/ports/security/nss/patches/patch-mozilla_security_coreconf_OpenBSD_mk?rev=1.1.1.1

Feel free to adapt any other changes from OpenBSD ports repository to shrink our patches!

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

I'm in favor of shrinking downstream patches!  
The way to do that, IMO, is to contribute the patches upstream.

Shall I interpret the two patch files cited in comment 3 as contributions
to moilla's security/coreconf ?
If so, to whom should those patches be credited?

Comment 5

[Martynas Venckus]

Sure.

Credited? Dunno, to OpenBSD team perhaps. There are quite a lot developers working on this port.

Comment 6

[Martynas Venckus]

Attachment: OpenBSD.mk with more hunks

Peter Strömberg submitted attachment 143032 [details] [diff] [review] in bug 236599 which got r=wtc.

Please see:
https://bugzilla.mozilla.org/show_bug.cgi?id=236599#c30
https://bugzilla.mozilla.org/show_bug.cgi?id=236599#c33

Also, use -Bsymbolic when linking nss to avoid symbol conflicts with libraries
that include OpenSSL.
We've got zlib in base, i've enabled that too.

Nelson, is there any reason for not committing it upstream? Every mozilla port uses this configuration (with changes to DLL_SUFFIX and OS_LIBS which i didn't include here).

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 255473 [details] [diff] [review]
OpenBSD.mk with more hunks

Since the openBSD community wants it, This seems fine to me.  
Wan-Teh, do you concur?

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 254723 [details] [diff] [review]
from comment 0

I gave r+ to the other patch for this bug.

Comment 9

[Wan-Teh Chang]

Comment on attachment 255473 [details] [diff] [review]
OpenBSD.mk with more hunks

The CPU_ARCH change is fine.

We should not take the DEFAULT_COMPILER, CC, and CCC changes.
I believe those changes are no longer necessary after bug 93206
was fixed.

The DSO_LDOPTS change is fine.

The -Bsymbolic change should be made for all platforms that support
that linker flag.  I believe that it is sufficient to only use
-Bsymbolic when we build libfreebl3.so.  This can be done by adding
OpenBSD to the following list in mozilla/security/nss/lib/freebl/Makefile:

ifeq (,$(filter-out BSD_OS FreeBSD Linux NetBSD, $(OS_TARGET)))
    MKSHLIB += -Wl,-Bsymbolic
endif

See bug 323977 for why -Bsymbolic is needed for libfreebl3.so.

The ZLIB change is fine.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

> I believe those changes are no longer necessary after bug 93206 was fixed.

The fix for bug 93206 means that when NSS is built as part of the browser
build, these make variables will get the right values, but it doesn't help
stand-alone NSS builds on OpenBSD.  I think the patch in attachment 255473 [details] [diff] [review]
was intended to fix stand-alone builds.  (yes?)

Comment 11

[Martynas Venckus]

OK. Verified the new patches i'm attaching work fine with standalone builds.

Comment 12

[Martynas Venckus]

Attachment: patch: mozilla/security/nss/lib/freebl/Makefile

Comment 13

[Martynas Venckus]

Attachment: patch-mozilla_security_coreconf_OpenBSD_mk

Uhm, we use our own shared library versioning in our ports tree. Could we use SO_VERSION in the environment to pass it to the libnss?

Comment 14

[Martynas Venckus]

We also pass CC and CXX. Why not do something like:

CC		       ?= gcc
DEFAULT_COMPILER	= ${CC}

Comment 15

[Martynas Venckus]

Attachment: patch v2: mozilla/security/coreconf/OpenBSD.mk

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 255891 [details] [diff] [review]
patch: mozilla/security/nss/lib/freebl/Makefile

r=nelson for this change, for the trunk (NSS 3.12)

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 260689 [details] [diff] [review]
patch v2: mozilla/security/coreconf/OpenBSD.mk

I have no way to test this patch.
Since it is only for files used only on OpenBSD,
if the OpenBSD community is in agreement concerning this patch, then I have no objecttion to it.  
r=nelson for the trunk.
Waiting for Wan-Teh's SR.

Comment 18

[Martynas Venckus]

Nelson, i'm also asking for attachment 255891 [details] [diff] [review] inclusion in nss 3.11, too, because it won't build (crash while building).

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

Martynas, Then you need to request a second review from another NSS module 
owner or peer, as you did for the other patch.

Comment 20

[Wan-Teh Chang]

Comment on attachment 255891 [details] [diff] [review]
patch: mozilla/security/nss/lib/freebl/Makefile

r=wtc.

Comment 21

[Wan-Teh Chang]

Comment on attachment 260689 [details] [diff] [review]
patch v2: mozilla/security/coreconf/OpenBSD.mk

r=wtc.  Please submit a new patch with the following change.

>-DEFAULT_COMPILER	= gcc
>-CC			= gcc
>-CCC			= g++
>+CC			?= gcc
>+CXX			?= g++
>+DEFAULT_COMPILER	= ${CC}
>+CCC			= ${CXX}
> RANLIB			= ranlib

Don't change the DEFAULT_COMPILER line.  It should be set to gcc,
the default value of CC.

Note: this patch introduces the use of ?= and the dependency on the
CC and CXX environment variables in NSS.  Previously, we need to
override CC and CCC on the make command line like this:

  make CC=/usr/local/bin/gcc CCC=/usr/local/bin/g++

I think it is OK to stick with this method, but I don't want to
be bogged down by this minor issue.

Comment 22

[Martynas Venckus]

(In reply to comment #21)
> (From update of attachment 260689 [details] [diff] [review])
> r=wtc.  Please submit a new patch with the following change.
> 
> >-DEFAULT_COMPILER	= gcc
> >-CC			= gcc
> >-CCC			= g++
> >+CC			?= gcc
> >+CXX			?= g++
> >+DEFAULT_COMPILER	= ${CC}
> >+CCC			= ${CXX}
> > RANLIB			= ranlib
> 
> Don't change the DEFAULT_COMPILER line.  It should be set to gcc,
> the default value of CC.
> 
> Note: this patch introduces the use of ?= and the dependency on the
> CC and CXX environment variables in NSS.  Previously, we need to
> override CC and CCC on the make command line like this:
> 
>   make CC=/usr/local/bin/gcc CCC=/usr/local/bin/g++
> 
> I think it is OK to stick with this method, but I don't want to
> be bogged down by this minor issue.
> 

I think you are right and i'm fine with you not taking this change; the build should respect CC/CXX environment variables. Not only for OpenBSD.

Comment 23

[Martynas Venckus]

s/enh/normal/, since nss won't even build by default.
I'm new to your nss/nspr checking in process.  What should i do now to get the two diffs committed?

Comment 24

[Nelson Bolyard (seldom reads bugmail)]

Thanks for the reminder.

Bug 370062 – nss build changes for OpenBSD, r=nelson,wtc
patches contributed by Martynas Venckus <[e-mail]>

On trunk:
coreconf/OpenBSD.mk;     new revision: 1.5; previous revision: 1.4
nss/lib/freebl/Makefile; new revision: 1.89; previous revision: 1.88

On 3.11 branch:
coreconf/OpenBSD.mk;     new revision: 1.4.28.1; previous revision: 1.4
nss/lib/freebl/Makefile; new revision: 1.70.2.13; previous revision: 1.70.2.12

Comment 25

[Nelson Bolyard (seldom reads bugmail)]

Attachment: final patch, as checked in

Comment 26

[Wan-Teh Chang]

Comment on attachment 263899 [details] [diff] [review]
final patch, as checked in

The value of DEFAULT_COMPILER should be a
constant, 'gcc'.  (The DEFAULT_COMPILER is
only used by our internal binary distributions.
Since we don't have any OpenBSD binary
distributions, this issue is moot.)