322450 - TARGET_XPCOM_ABI not set in solaris builds

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Leon Sha]

http://developer.mozilla.org/en/docs/TARGET_XPCOM_ABI
on Solaris this is never set correctly.
Issuing the following command in the javascript console:
Components.classes["@mozilla.org/xre/app-info;1"].getService(Components.interfaces.nsIXULRuntime).XPCOMABI
There should be some result there.

Comment 1

[Leon Sha]

Attachment: Patch

Comment 2

[Benjamin Smedberg]

Comment on attachment 207595 [details] [diff] [review]
Patch

We do not want to set TARGET_XPCOM_ABI unless target-compiler-abi is set.

Comment 3

[Jean-Christophe Collet]

(In reply to comment #2)
> (From update of attachment 207595 [details] [diff] [review] [edit])
> We do not want to set TARGET_XPCOM_ABI unless target-compiler-abi is set.
> 

Why not? the documentation at http://developer.mozilla.org/en/docs/TARGET_XPCOM_ABI specifically says that TARGET_COMPILER_ABI should be ignored for all but the 5 listed compilers:
"For other compilers, this value is ommited, along with the preceding dash."

Also, it even says: 
"Thunderbird built with the Sun C++ Compiler for the SPARC processor would have XPCOM ABI of sparc"

This is important for extensions that include some native code since Solaris (SunOS) exists in at least two architectures: sparc and x86

Comment 4

[Ilya Konstantinov]

I admit having written that piece of documentation (the TARGET_XPCOM_ABI page at MDC) without extensive research :)

At any rate, if we avoid setting TARGET_XPCOM_ABI on Solaris (just because target-compiler-abi isn't set), then me, as an extension developer (whose extension requires a binary component) cannot make a multiplatform XPI with Solaris inside, since I have no way to distinct x86 Solaris, x86-64 Solaris and Sparc Solaris.

Comment 5

[Benjamin Smedberg]

The documents were based on the code, not the other way around. Every value should have an CPU-compiler combination. I have updated the doc to make it clear that there should always be both values.

I suggest using

"sparc-sunc" or something similar to identify your CPU/compiler combination.

Comment 6

[Jean-Christophe Collet]

Attachment: patch to configure

K, sounds fair enough.
Here is a new proposed fix that I just verified.
Note that on Solaris 'uname -m' returns 'i86pc' for x86 and 'sun4u' for sparc, which explains my first set of changes (to set CPU_ARCH).

I tested the changes on Solaris Sparc and Solaris x86, both with Sun C and gcc, and now TARGET_XPCOM_ABI is set to either of these 4 values:
 x86-sunc
 x86-gcc3
 sparc-sunc
 sparc-gcc3

Is this satisfactory?

Comment 7

[Benjamin Smedberg]

Comment on attachment 207615 [details] [diff] [review]
patch to configure

Yes, excellent! Please provide this as a -u8 patch to configure.in so that I can review and land it.

Comment 8

[Jean-Christophe Collet]

Attachment: configure.in patch

here is the proposed fix to configure.in (genereated as diff -u

Comment 9

[Benjamin Smedberg]

Comment on attachment 207620 [details] [diff] [review]
configure.in patch

r=me, I'll land this.

Comment 10

[Benjamin Smedberg]

Fixed on trunk. I don't see any reason we can't get this on the 1.8 branch as well, and probably 1.8.0.2 if there aren't any regressions on trunk.

Comment 11

[Ilya Konstantinov]

Mike, you might want to check out with whoever at IBM builds Firefox for IBM's platforms to ensure this bug doesn't repeat there.

Comment 12

[Ilya Konstantinov]

Any chance of it going into TB1.5 final? I'd really hate us to miss the opportunity brought up with the platform-specific directories feature by missing such a simple thing.

Comment 13

[Benjamin Smedberg]

No, the first build this is likely to get approved for is ffox/tbird 1.5.0.2

Comment 14

[Phil Ringnalda (:philor)]

*** Bug 334270 has been marked as a duplicate of this bug. ***

Comment 15

[Phil Ringnalda (:philor)]

bsmedberg, were you still planning on landing this on branches, including the s/CPU_ARCH_TEST/OS_TEST/ you caught on checkin?

Comment 16

[Nian Liu(n/a in a long time)]

Comment on attachment 207620 [details] [diff] [review]
configure.in patch

sun handicap concerning extension team really need that. thanks a lot

Comment 17

[Axel Hecht]

This is most definitly not 1.8.0.3 material, but you may want to try
1.8.0.4.

Comment 18

[Nian Liu(n/a in a long time)]

Comment on attachment 207620 [details] [diff] [review]
configure.in patch

moved to 1.8.0.4 addressed by axel

Comment 19

[Daniel Veditz [:dveditz]]

Comment on attachment 207620 [details] [diff] [review]
configure.in patch

Needs 1.8.1 approval and landing first.

Comment 20

[Nian Liu(n/a in a long time)]

Benjamin,

pls. help check the patch in 1.8.1

Comment 21

[Daniel Veditz [:dveditz]]

Comment on attachment 207620 [details] [diff] [review]
configure.in patch

Too late in this cycle, please try early for next release.

Comment 22

[Benjamin Smedberg]

Fixed on 1.8 branch.

Comment 23

[Nian Liu(n/a in a long time)]

Benjamin,

how about 1.8.0.5?

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Any chance we could get this (i.e., what was checked in, which is a little different from the patch in the bug, IIRC) in for 1.8.0.5, so that we could ship extensions with binary components for platforms like Linux x86_64?

Comment 25

[Benjamin Smedberg]

dbaron, are you thinking of bug 335143?

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

No, I'm thinking of the change made when this patch was checked in to the trunk and 1.8 branch that's not part of the patch above:

 case "$OS_TEST" in
 [...]
 x86_64 | sparc | ppc | ia64)
-    CPU_ARCH="$CPU_ARCH_TEST"
+    CPU_ARCH="$OS_TEST"
     ;;
 esac

which actually caused $CPU_ARCH to be set on those 4 platforms, which allowed TARGET_XPCOM_ABI to be non-empty in the build system.

Maybe that patch is needed too, though.

Comment 27

[Daniel Veditz [:dveditz]]

Sounds like we need a new patch for the 1.8.0 branch?

Comment 28

[Daniel Veditz [:dveditz]]

Comment on attachment 207620 [details] [diff] [review]
configure.in patch

minusing for 1.8.0.x, it sounds like we need a new patch for the branches

Comment 29

[Daniel Veditz [:dveditz]]

This isn't really a blocking bug. Would approve a branch patch.

Comment 30

[Benjamin Smedberg]

Attachment: 180 branch patch

Comment 31

[Daniel Veditz [:dveditz]]

Comment on attachment 225422 [details] [diff] [review]
180 branch patch

approved for 1.8.0 branch, a=dveditz for drivers

Comment 32

[Benjamin Smedberg]

Fixed on MOZILLA_1_8_0_BRANCH

Comment 33

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Note that you didn't commit to configure as well, and it's not auto-updated on MOZILLA_1_8_0_BRANCH.  I committed (yesterday) an updated configure (and checked that autoconf-2.13 on my machine matches what is auto-generated on the trunk and the 1.8 branch, and that the diffs to configure were only expected diffs).