Closed Bug 1603398 Opened 4 years ago Closed 4 years ago

Fix nsinstall.c build failure on RHEL 7

Categories

(NSS :: Build, defect, P1)

Product:
NSS
Version:
3.48
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED INVALID

People

(Reporter: giulio.benetti, Assigned: giulio.benetti)

Details

Attachments

(1 file)

0001-Bug-1603398-Fix-nsinstall.c-build-failure-on-RHEL-7.patch
883 bytes, patch
Details | Diff | Splinter Review

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/79.0.3945.79 Safari/537.36

Steps to reproduce:

Build with RHEL 7.4

Actual results:

It fails with:
/usr/bin/gcc -o Linux2.6_aarch64_aarch64_be-linux-gnu-gcc_glibc_PTH_64_DBG.OBJ/nsinstall.o -c -std=c99 -g -O2 -I/home/br-user/fc68ddc3e479306f50e2d377133ba3bc3706eb06/output/host/include -Wall -Wshadow -DNSS_NO_GCC48 -DXP_UNIX -DDEBUG -UNDEBUG -D_DEFAULT_SOURCE -D_BSD_SOURCE -D_POSIX_SOURCE -D_REENTRANT -DNSS_NO_INIT_SUPPORT -DUSE_UTIL_DIRECTLY -DNO_NSPR_10_SUPPORT -DSSL_DISABLE_DEPRECATED_CIPHER_SUITE_NAMES -I/home/br-user/fc68ddc3e479306f50e2d377133ba3bc3706eb06/output/host/aarch64_be-buildroot-linux-gnu/sysroot/usr/include/nspr -I/home/br-user/fc68ddc3e479306f50e2d377133ba3bc3706eb06/output/build/libnss-3.48/dist/include -I../../../dist/public/coreconf -I../../../dist/private/coreconf nsinstall.c
nsinstall.c: In function ‘main’:
nsinstall.c:201:5: warning: implicit declaration of function ‘getopt’ [-Wimplicit-function-declaration]
while ((opt = getopt(argc, argv, "C:DdlL:Rm:o:g:t")) != EOF) {
^
nsinstall.c:203:20: error: ‘optarg’ undeclared (first use in this function)
case 'C': cwd = optarg; break;
^
nsinstall.c:203:20: note: each undeclared identifier is reported only once for each function it appears in
nsinstall.c:225:13: error: ‘optind’ undeclared (first use in this function)
argc -= optind;
^

Assignee: nobody → giulio.benetti
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Priority: -- → P1
Comment on attachment 9115418 [details] [diff] [review]
0001-Bug-1603398-Fix-nsinstall.c-build-failure-on-RHEL-7.patch

Daiki, can you review this one?
Attachment #9115418 - Flags: review?(dueno)
Comment on attachment 9115418 [details] [diff] [review]
0001-Bug-1603398-Fix-nsinstall.c-build-failure-on-RHEL-7.patch

Review of attachment 9115418 [details] [diff] [review]:
-----------------------------------------------------------------

At first glance, this doesn't look correct to me, given that those flags are properly emitted on my local build even without this change.
Giulio, how do you build NSS on RHEL 7.4? If you override CFLAGS, I would suggest using XCFLAGS instead.

Hi Daiki,

(In reply to Daiki Ueno [:ueno] from comment #3)

Comment on attachment 9115418 [details] [diff] [review]
0001-Bug-1603398-Fix-nsinstall.c-build-failure-on-RHEL-7.patch

Review of attachment 9115418 [details] [diff] [review]:

At first glance, this doesn't look correct to me, given that those flags are
properly emitted on my local build even without this change.

Can you check id during NSS building of nsintall.c, -DLINUX is emitted correctly?
What happens in both Ubuntu and RHEL is that -DLINUX is not emitted,
so in RHEL gcc doesn't include <getopt.h> and miss getop(), getind etc.
But in Ubuntu(and in most of distros) it doesn't include <getopt.h> in nsinstall.c
but it indirectly includes it through <unistd.h> which includes <bits/getopt_posix.h>
without the need of including <getopt.h>. This is why it works on many distros.

OS_CFLAGS, as I understand, are CFLAGS for target-only NSS, and indeed when
building on host they are not emitted like -DLINUX.

Best regards
Giulio Benetti

Giulio, how do you build NSS on RHEL 7.4? If you override CFLAGS, I would
suggest using XCFLAGS instead.

I tried using XCFLAGS instead of CFLAGS, and it doesn't change. I reproduced the issue reported by Giulio. Here is how it fails:

/usr/bin/gcc -o Linux2.6_arm_arm-linux-gcc.br_real_glibc_PTH_DBG.OBJ/nsinstall.o -c -std=c99 -g -O2 -I/root/buildroot/output/host/include -Wall -Wshadow -DNSS_NO_GCC48 -DXP_UNIX -DDEBUG -UNDEBUG -D_DEFAULT_SOURCE -D_BSD_SOURCE -D_POSIX_SOURCE -D_REENTRANT -DNSS_NO_INIT_SUPPORT -DUSE_UTIL_DIRECTLY -DNO_NSPR_10_SUPPORT -DSSL_DISABLE_DEPRECATED_CIPHER_SUITE_NAMES -I/root/buildroot/output/host/arm-buildroot-linux-uclibcgnueabi/sysroot/usr/include/nspr -I/root/buildroot/output/build/libnss-3.48/dist/include -I../../../dist/public/coreconf -I../../../dist/private/coreconf -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64 -Os nsinstall.c
nsinstall.c: In function 'main':
nsinstall.c:201:5: warning: implicit declaration of function 'getopt' [-Wimplicit-function-declaration]
while ((opt = getopt(argc, argv, "C:DdlL:Rm:o:g:t")) != EOF) {
^
nsinstall.c:203:20: error: 'optarg' undeclared (first use in this function)
case 'C': cwd = optarg; break;
^
nsinstall.c:203:20: note: each undeclared identifier is reported only once for each function it appears in
nsinstall.c:225:13: error: 'optind' undeclared (first use in this function)
argc -= optind;
^

And indeed, adding just -DLINUX to the build command line makes it work:

/usr/bin/gcc -o Linux2.6_arm_arm-linux-gcc.br_real_glibc_PTH_DBG.OBJ/nsinstall.o -c -std=c99 -g -O2 -I/root/buildroot/output/host/include -Wall -Wshadow -DNSS_NO_GCC48 -DXP_UNIX -DDEBUG -UNDEBUG -D_DEFAULT_SOURCE -D_BSD_SOURCE -D_POSIX_SOURCE -D_REENTRANT -DNSS_NO_INIT_SUPPORT -DUSE_UTIL_DIRECTLY -DNO_NSPR_10_SUPPORT -DSSL_DISABLE_DEPRECATED_CIPHER_SUITE_NAMES -DLINUX -I/root/buildroot/output/host/arm-buildroot-linux-uclibcgnueabi/sysroot/usr/include/nspr -I/root/buildroot/output/build/libnss-3.48/dist/include -I../../../dist/public/coreconf -I../../../dist/private/coreconf -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64 -Os nsinstall.c

Daiki, can you provide another round of feedback? I'd like to get this resolved in NSS 3.50 if possible. Thanks!

Flags: needinfo?(dueno)

Sorry for the delay. For me the question is still how you build NSS exactly. In Fedora's armv7hl, the emitted command line is:

cc -o Linux5.3_arm_cc_glibc_PTH_OPT.OBJ/nsinstall.o -c -std=c99 -O2 -fPIC   -pipe -ffunction-sections -fdata-sections -DHAVE_STRERROR -DLINUX -Dlinux -Wall -Wshadow -Werror -DXP_UNIX -UDEBUG -DNDEBUG -D_DEFAULT_SOURCE -D_BSD_SOURCE -D_REENTRANT -DUSE_UTIL_DIRECTLY -DNO_NSPR_10_SUPPORT -DSSL_DISABLE_DEPRECATED_CIPHER_SUITE_NAMES -I/usr/include/nspr4  -iquote ../../../dist/Linux5.3_arm_cc_glibc_PTH_OPT.OBJ/../public/nss -iquote ../../../dist/Linux5.3_arm_cc_glibc_PTH_OPT.OBJ/../private/nss -I../../../dist/Linux5.3_arm_cc_glibc_PTH_OPT.OBJ/include -I../../../dist/public/coreconf -I../../../dist/private/coreconf -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions -fstack-protector-strong -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -march=armv7-a -mfpu=vfpv3-d16 -mtune=generic-armv7-a -mabi=aapcs-linux -mfloat-abi=hard nsinstall.c

which contains -DLINUX as you see.

Flags: needinfo?(dueno)

Hi Daiki,

the only way I can reproduce it is building with Buildroot with only:
BR2_PACKAGE_LIBNSS=y
in .config

Can you please build using Buildroot on x86?

Here is how:

git clone https://git.busybox.net/buildroot

cd buildroot

echo BR2_PACKAGE_LIBNSS=y > configs/libnss_defconfig

make libnss_defconfig

make libnss

I don't know another way to reproduce the problem.

Thanks in advance
Giulio Benetti(In reply to Daiki Ueno [:ueno] from comment #7)

Sorry for the delay. For me the question is still how you build NSS exactly. In Fedora's armv7hl, the emitted command line is:

cc -o Linux5.3_arm_cc_glibc_PTH_OPT.OBJ/nsinstall.o -c -std=c99 -O2 -fPIC   -pipe -ffunction-sections -fdata-sections -DHAVE_STRERROR -DLINUX -Dlinux -Wall -Wshadow -Werror -DXP_UNIX -UDEBUG -DNDEBUG -D_DEFAULT_SOURCE -D_BSD_SOURCE -D_REENTRANT -DUSE_UTIL_DIRECTLY -DNO_NSPR_10_SUPPORT -DSSL_DISABLE_DEPRECATED_CIPHER_SUITE_NAMES -I/usr/include/nspr4  -iquote ../../../dist/Linux5.3_arm_cc_glibc_PTH_OPT.OBJ/../public/nss -iquote ../../../dist/Linux5.3_arm_cc_glibc_PTH_OPT.OBJ/../private/nss -I../../../dist/Linux5.3_arm_cc_glibc_PTH_OPT.OBJ/include -I../../../dist/public/coreconf -I../../../dist/private/coreconf -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions -fstack-protector-strong -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -march=armv7-a -mfpu=vfpv3-d16 -mtune=generic-armv7-a -mabi=aapcs-linux -mfloat-abi=hard nsinstall.c

which contains -DLINUX as you see.

Hi Daiki,

the only way I can reproduce it is building with Buildroot with only:
BR2_PACKAGE_LIBNSS=y
in .config

Can you please build using Buildroot on x86?

Here is how:

git clone https://git.busybox.net/buildroot

cd buildroot

echo BR2_PACKAGE_LIBNSS=y > configs/libnss_defconfig

make libnss_defconfig

make libnss

I don't know another way to reproduce the problem.

Thanks in advance
Giulio Benetti

I tried and could reproduce the issue. However, I'm not sure if it should be fixed in NSS side.
To my understanding, the problem is that the buildroot project's infrastructure forces to put CFLAGS on the make command line, which overrides CFLAGS that NSS Makefiles calculate. My suggestion to use XCFLAGS instead was to stop using CFLAGS for supplying custom compiler options. Wouldn't it be possible with the buildroot packaging?

(In reply to Daiki Ueno [:ueno] from comment #9)

I tried and could reproduce the issue. However, I'm not sure if it should be fixed in NSS side.
To my understanding, the problem is that the buildroot project's infrastructure forces to put CFLAGS on the make command line, which overrides CFLAGS that NSS Makefiles calculate. My suggestion to use XCFLAGS instead was to stop using CFLAGS for supplying custom compiler options. Wouldn't it be possible with the buildroot packaging?

I try to dig deeper and reach the keystone :-)
Giulio

(In reply to Daiki Ueno [:ueno] from comment #9)

I tried and could reproduce the issue. However, I'm not sure if it should be fixed in NSS side.
To my understanding, the problem is that the buildroot project's infrastructure forces to put CFLAGS on the make command line, which overrides CFLAGS that NSS Makefiles calculate. My suggestion to use XCFLAGS instead was to stop using CFLAGS for supplying custom compiler options. Wouldn't it be possible with the buildroot packaging?

I did try to use XCFLAGS, and it didn't work. See my "Comment 5" from me.

I dug it a bit further and realized that the interfering variable is NATIVE_FLAGS, which always has higher precedence in nsinstall build:
https://hg.mozilla.org/projects/nss/file/tip/coreconf/nsinstall/Makefile#l32

If you remove this setting from the buildroot's libnss.mk, the build picks up the correct flags:

diff --git a/package/libnss/libnss.mk b/package/libnss/libnss.mk
index 68389d46fc..c0f48921a0 100644
--- a/package/libnss/libnss.mk
+++ b/package/libnss/libnss.mk
@@ -68,7 +68,7 @@ define LIBNSS_BUILD_CMDS
                SOURCE_MD_DIR=$(@D)/$(LIBNSS_DISTDIR) \
                DIST=$(@D)/$(LIBNSS_DISTDIR) \
                CHECKLOC= \
-               $(LIBNSS_BUILD_VARS) NATIVE_FLAGS="$(HOST_CFLAGS)"
+               $(LIBNSS_BUILD_VARS)
 endef
 
 define LIBNSS_INSTALL_STAGING_CMDS

Here is the log:

[...]
/usr/lib64/ccache/gcc -o Linux2.6_x86_i586-buildroot-linux-uclibc-gcc.br_real_glibc_PTH_DBG.OBJ/nsinstall.o -c -std=c99 -g -fPIC -Di386  -pipe -ffunction-sections -fdata-sections -DHAVE_STRERROR -DLINUX -Dlinux -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -D_FILE_OFFSET_BITS=64  -Os   -Wall -Wshadow -DNSS_NO_GCC48 -DXP_UNIX -DDEBUG -UNDEBUG -D_DEFAULT_SOURCE -D_BSD_SOURCE -D_POSIX_SOURCE -D_REENTRANT -DNSS_NO_INIT_SUPPORT -DUSE_UTIL_DIRECTLY -DNO_NSPR_10_SUPPORT -DSSL_DISABLE_DEPRECATED_CIPHER_SUITE_NAMES -I/home/dueno/devel/buildroot/output/host/i586-buildroot-linux-uclibc/sysroot/usr/include/nspr -I/home/dueno/devel/buildroot/output/build/libnss-3.48/dist/include -I../../../dist/public/coreconf -I../../../dist/private/coreconf  nsinstall.c

If you do want to pass $(HOST_CFLAGS), again I would suggest using XCFLAGS instead.

(In reply to Daiki Ueno [:ueno] from comment #12)

If you do want to pass $(HOST_CFLAGS), again I would suggest using XCFLAGS instead.

Using XCFLAGS in place of NATIVE_FLAGS works because XCFLAGS is appended to CFLAGS
in coreconf/command.mk:

CFLAGS        = $(OPTIMIZER) $(OS_CFLAGS) $(WARNING_CFLAGS) $(XP_DEFINE) \
                $(DEFINES) $(INCLUDES) $(XCFLAGS)

And it worked with DEFINES += -DLINUX -DLinux too, because DEFINES is passed to CFLAGS too above.

NATIVE_FLAGS if present override OS_CFLAGS in coreconf/nsintall/Makefile, that are used to set CFLAGS too(see above):

ifdef NATIVE_FLAGS
OS_CFLAGS=$(NATIVE_FLAGS)
endif

So NATIVE_FLAGS is meant to build nsinstall and mkdepend too. IMHO the best solution is to still use "NATIVE_FLAGS=$(HOST_CFLAGS)",
but append -DLINUX to NATIVE_FLAGS too, since we're building a host program, not a target one.

And this is correct to me because OS_TEST is Linux and Host is Linux too in our case. But if one builds for target Linux on another Host OS,
passing -DLINUX when building host program(nsintall) is wrong.

So I'd go for NATIVE_FLAGS="$(HOST_CFLAGS) -DLINUX".

Best regards

I suppose that's the right solution for this, according to Bob's comment on bug 104541 comment 2.
In the patch you changed DEFINES in Linux.mk, but the file shall be loaded when the target platform (not the host) is Linux. So the host is responsible for supplying proper compiler and flags through NATIVE_*.

(In reply to Daiki Ueno [:ueno] from comment #14)

I suppose that's the right solution for this, according to Bob's comment on bug 104541 comment 2.
In the patch you changed DEFINES in Linux.mk, but the file shall be loaded when the target platform (not the host) is Linux. So the host is responsible for supplying proper compiler and flags through NATIVE_*.

Agree, so we mark this as RESOLVED->INVALID and I'm going to send 2 patches to Buildroot for reverting and fixing that way.

Thank you
Best regards

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → INVALID
Attachment #9115418 - Flags: review?(dueno)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: