Last Comment Bug 330628 - coreconf/Linux.mk should _not_ default to x86 but result in an error if host is not recognized
: coreconf/Linux.mk should _not_ default to x86 but result in an error if host ...
Status: RESOLVED FIXED
:
Product: NSS
Classification: Components
Component: Build (show other bugs)
: unspecified
: x86 Linux
: -- normal (vote)
: 3.12.1
Assigned To: Mike Hommey [:glandium]
:
:
Mentors:
Depends on:
Blocks: 448792
  Show dependency treegraph
 
Reported: 2006-03-15 16:25 PST by Martin Michlmayr
Modified: 2008-08-14 11:55 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch for the issue (482 bytes, patch)
2006-03-16 05:00 PST, Mike Hommey [:glandium]
no flags Details | Diff | Splinter Review
New patch (574 bytes, patch)
2006-03-16 09:32 PST, Mike Hommey [:glandium]
rrelyea: review+
Details | Diff | Splinter Review
New patch v2: use i%86 (643 bytes, patch)
2008-08-14 11:06 PDT, Wan-Teh Chang
kaie: review+
Details | Diff | Splinter Review

Description Martin Michlmayr 2006-03-15 16:25:39 PST
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] 2006-03-16 05:00:49 PST
Created attachment 215258 [details] [diff] [review]
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 2006-03-16 07:28:00 PST
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 rrelyea@redhat.com to review patches.
Comment 3 Mike Hommey [:glandium] 2006-03-16 07:34:55 PST
(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 2006-03-16 09:12:46 PST
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] 2006-03-16 09:28:45 PST
(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] 2006-03-16 09:32:27 PST
Created attachment 215288 [details] [diff] [review]
New patch

Here it is
Comment 7 Robert Relyea 2006-03-16 13:15:43 PST
Comment on attachment 215288 [details] [diff] [review]
New patch

r+= relyea
Comment 8 timeless 2006-07-15 18:56:08 PDT
perhaps this could be checked in?
Comment 9 Reed Loden [:reed] (use needinfo?) 2007-10-11 23:38:41 PDT
I don't know the approval requirements for NSS.

Bob, can you check this in?
Comment 10 Nelson Bolyard (seldom reads bugmail) 2007-10-12 09:11:06 PDT
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] (use needinfo?) 2008-04-26 19:41:23 PDT
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) 2008-04-27 13:38:44 PDT
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] (use needinfo?) 2008-07-12 14:13:24 PDT
wtc, think you could land this patch, too? It's been waiting for somebody to commit it for a very long time. Thanks!
Comment 15 Nelson Bolyard (seldom reads bugmail) 2008-08-14 04:29:46 PDT
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] 2008-08-14 05:12:53 PDT
backed out.
Comment 17 Kai Engert (:kaie) 2008-08-14 06:04:01 PDT
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) 2008-08-14 06:09:27 PDT
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 2008-08-14 07:26:26 PDT
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 2008-08-14 11:06:28 PDT
Created attachment 333791 [details] [diff] [review]
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) 2008-08-14 11:51:45 PDT
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) 2008-08-14 11:55:53 PDT
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

Note You need to log in before you can comment on or make changes to this bug.