330628 - coreconf/Linux.mk should _not_ default to x86 but result in an error if host is not recognized

Status: RESOLVED FIXED

(NSS :: Build, defect)

Description

[Martin Michlmayr]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.12) Gecko/20050922 Firefox/1.0.7 (Debian package 1.0.7-1)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.12) Gecko/20050922 Firefox/1.0.7 (Debian package 1.0.7-1)

At the moment, the host detection script coreconf/Linux.mk falls back to x86 when it doesn't know about the host.  This is a really, really bad idea.  I just tried to compiler mozilla-thunderboard and xulrunner on mips64 (which coreconf/Linux.mk doesn't recognize at the moment; see bug 330626).  With mozilla-thunderbird, I got an error during linking because it couldn't find ssl3.  Looking at previous bug reports of mozilla-thunderbird in our (Debian) bug tracking system showed that someone else reported the same bug on AMD64 several months ago.  It was closed as unreproducible... only after I looked at the logs in full detail I figured out what was going on (i.e. that it was trying to compile for x86).

Assuming x86 as default is really harmful since it leads to really odd compilation errors.  Can you please make your build system more robust and simply generate an error when you cannot guess the type?  Then it's obvious what's wrong and people can submit a patch to add host detection for their target.



Reproducible: Always

Steps to Reproduce:
Compile any mozilla app on an architecture mozilla/security/coreconf/Linux.mk doesn't know about...
Actual Results:  
It will fail with really bizarre errors.


Expected Results:  
It should simply produce an error saying that the host is not recognized.


Debian Bug #356973: build system not robust, assumes x86 when arch unknown
http://bugs.debian.org/356973
Compilation of mozilla-thunderbird on mips64 fails wil a ssl3 not found linking error.

Debian Bug #302657: missing dependency: ssl3
http://bugs.debian.org/302657
Closed as unreproducible: turns out now that it is because at that time AMD64 was not detected as a host arch.

Debian Bug #357035: build system not robust, assumes x86 when arch unknown
http://bugs.debian.org/357035
Compilation of xulrunner on mips64 fails when trying to compile an x86 assembler file...

Comment 1

[Mike Hommey [:glandium]]

Attachment: patch for the issue

It seems that not providing the CPU_ARCH value is just fine, the freebl/mpi part just builds its C counterpart then. I haven't tested if the lib still works correctly, but it builds fine, without using x86 asm ; at least for freebl/mpi. For other directories, that would need some more testing with unsupported architecture.

Comment 2

[Wan-Teh Chang]

Comment on attachment 215258 [details] [diff] [review]
patch for the issue

>+ifeq (,$(filter-out i686 i586 i486 i386,$(OS_TEST)))

The filter and filter-out functions actually accept
patterns as input, so you can replace
i686 i586 i486 i386 by i%86.

I am out of the office for two weeks.  Please ask
my colleague [e-mail] to review patches.

Comment 3

[Mike Hommey [:glandium]]

(In reply to comment #2)
> (From update of attachment 215258 [details] [diff] [review] [edit])
> >+ifeq (,$(filter-out i686 i586 i486 i386,$(OS_TEST)))
> 
> The filter and filter-out functions actually accept
> patterns as input, so you can replace
> i686 i586 i486 i386 by i%86.

I actually did that intentionally, just to be sure no impossible4286 would match.

Comment 4

[Robert Relyea]

In the default case:

I would like to see the patch do something like set CPU_ARCH to $(shell uname -m) for sanity sake.

We should also set OS_REL_CFLAGS to "-DLINUX1_2 -D_XOPEN_SOURCE", though I suspect NSS doesn't really care (I think they are hold overs from other coreconf products).

Other than that, I think the patch is fine.

bob

Comment 5

[Mike Hommey [:glandium]]

(In reply to comment #4)
> In the default case:
> 
> I would like to see the patch do something like set CPU_ARCH to $(shell uname
> -m) for sanity sake.

OS_TEST is already set to $(shell uname -m), so we could just assign CPU_ARCH to OS_TEST.

> We should also set OS_REL_CFLAGS to "-DLINUX1_2 -D_XOPEN_SOURCE", though I
> suspect NSS doesn't really care (I think they are hold overs from other
> coreconf products).

Will attach updated patch soon.

Comment 6

[Mike Hommey [:glandium]]

Attachment: New patch

Here it is

Comment 7

[Robert Relyea]

Comment on attachment 215288 [details] [diff] [review]
New patch

r+= relyea

Comment 8

[timeless]

perhaps this could be checked in?

Comment 9

[Reed Loden [:reed]]

I don't know the approval requirements for NSS.

Bob, can you check this in?

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

This patch has a better-than-average chance of breaking Tinderbox and nightly
builds on Linux.  

So, Before this is checked in, notice should be sent to Christophe and Slavo, 
so they can watch those, and make any necessary adjustments.  I suggest doing 
a checkin on Monday through Thursday, not Friday, Saturday or Sunday, 
so that if Tinderbox breaks, it won't be broken all weekend.

Comment 11

[Reed Loden [:reed]]

Nelson, neither Gavin nor I have access to the restricted NSS partition to land this... So, you can either land it yourself or add us as members in the partition so we can land this on behalf of glandium.

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Reed, thanks for your reply.
I put that there as a reminder to my fellow NSS developers who have the 
principal responsibility for NSS on Linux.

I find that reviewed NSS patches sometimes fall off the radar without being
committed.  That's what happened to this patch.  And it's now so old that
I think it may need to be brought forward to the tip of the trunk and be
re-reviewed.  But whoever goes to check it in will discover that. :)

Comment 13

[Reed Loden [:reed]]

wtc, think you could land this patch, too? It's been waiting for somebody to commit it for a very long time. Thanks!

Comment 14

[Dão Gottwald [:dao]]

http://hg.mozilla.org/index.cgi/mozilla-central/rev/abfdc2e8da35

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

The primary "master" source of NSPR is still on cvs.  
If that "fix" was committed to hg and not CVS, then you've forked the tree.
Is that really your plan?

Comment 16

[Dão Gottwald [:dao]]

backed out.

Comment 17

[Kai Engert (:KaiE:)]

I'm testing it locally on my Linux machine and if it build, I will check it in.

In comment 10 had suggested we should give Christophe and Slavo a chance for comment, but they didn't, so I conclude we should go ahead and try it.

Dao, Reed, thanks for your attempt to help, but this needs to be landed in "upstream" NSS (a separate repository). It will make its way to the Mozilla apps once an updated NSS snapshot gets delivered.

Comment 18

[Kai Engert (:KaiE:)]

My local build completed fine.

I checked it in, but then backed myself out again. I forgot we're in a freeze towards NSS 3.12.1

We'll have to check this in next week, after the release is out.

Comment 19

[Wan-Teh Chang]

Comment on attachment 215288 [details] [diff] [review]
New patch

>+ifeq (,$(filter-out i686 i586 i486 i386,$(OS_TEST)))

We should use the pattern "i%86" instead of the
"i686 i586 i486 i386" list.

The gmake pattern "i%86" matches the shell pattern "i*86"
used in Mozilla and NSPR's configure.in scripts.

Comment 20

[Wan-Teh Chang]

Attachment: New patch v2: use i%86

Kai,

please check in this patch instead.

In comment 3, Mike Hommey said he intentionally listed i686,
i586, etc.  Here are the proofs that mozilla/nsprpub/configure.in
and mozilla/configure.in are already using the equivalent of i%86.

http://mxr.mozilla.org/nspr/source/nsprpub/configure.in#1471

1464     case "${target_cpu}" in
...
1471     i*86)
1472         AC_DEFINE(i386)
1473         PR_MD_ASFILES=os_Linux_x86.s
1474         ;;

http://mxr.mozilla.org/mozilla-central/source/configure.in#1844

1839     case "${target_cpu}" in
...
1844     i*86)
1845         USE_ELF_DYNSTR_GC=1
1846         MOZ_ENABLE_OLD_ABI_COMPAT_WRAPPERS=1
1847     ;;

Comment 21

[Kai Engert (:KaiE:)]

Comment on attachment 333791 [details] [diff] [review]
New patch v2: use i%86

r=kaie for Wan-Teh's change to the patch

Comment 22

[Kai Engert (:KaiE:)]

We have decided to take this patch for 3.12.1
Checked in

Checking in Linux.mk;
/cvsroot/mozilla/security/coreconf/Linux.mk,v  <--  Linux.mk
new revision: 1.34; previous revision: 1.33
done