Closed Bug 107976 Opened 23 years ago Closed 19 years ago

coreconf should allow CC and CCC to be overriden.

Categories

(NSS :: Build, enhancement, P1)

3.3.1
enhancement

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: wtc, Assigned: wtc)

References

Details

Attachments

(2 files, 1 obsolete file)

coreconf should allow CC and CCC (the C and C++ compilers) to be overriden. This will impose a new restriction on the values of CC and CCC and require some cleanup in coreconf to ensure that the new restriction is followed. The module (for example, PSM) that passes the values of CC and CCC to coreconf must also follow the restriction. The new restriction is that CC and CCC must be a single word, which is either the full pathname or the name of the compiler. CC and CC must not contain any compiler options. For example, some legal values of CC are: CC = gcc CC = /usr/local/bin/gcc CC = /home/wtc/bin/gcc-2.95.3 Some illegal values of CC are: CC = cc -Ae CC = cc -xarch=v9CC = gcc -pthread Please let me know if it is feasible to follow this restriction and whether additional restrictions are required.
Blocks: 93206
Correction: in the "Some illegal values of CC are" paragraph, the line CC = cc -xarch=v9CC = gcc -pthread should be two lines CC = cc -xarch=v9 CC = gcc -pthread
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → 3.4
What's the reason for the single word restriction? Building with CC='cc -xarch=v9' is a valid Mozilla build flag. We currently have a tinderboxes that building with CC='cc -xarch=v9'. I thought that the patch I attached to bug 93206 handled the multi-word case without a problem. I haven't tested the multi-word case recently though.
Chris Seawood wrote: > What's the reason for the single word restriction? > Building with CC='cc -xarch=v9' is a valid Mozilla > build flag. 1. The single word restriction must be imposed on the value of CC set by coreconf. I believe we all agree this is necessary. For example, if coreconf says CC = gcc -pthread and it gets overriden by CC = /home/wtc/bin/gcc on the command line, the critical -pthread flag will be lost. 2. So the only issue is whether the single word restriction should also be imposed on the value of CC passed to coreconf on the command line. As you pointed out, your patch handled the multi-word case without a problem, so it is in fact okay to pass a multi-word CC to coreconf. On the other hand, this restriction is not as bad as it seems. For the specific case of CC='cc -xarch=v9' that you raised, I am asking you to only pass the first word (that is, CC=cc) to coreconf. In this case, you will also be passing USE_64=1 to coreconf, which causes coreconf to add -xarch=v9 to CFLAGS. So in the end you will get exactly what you want. Allowing CC and CCC to be overridden will add a lot of flexibility to coreconf, but it will also open a lot of possible ways to break coreconf. I am concerned about the support cost down the road. This is why I want us to think this through and clearly state what can and cannot be done. My first proposal is intentionally more restrictive than necessary. I want to ensure that every step we take is safe. One question for you: is it more common to say CC='cc -xarch=v9' or CC=cc CFLAGS=-xarch=v9 to specify a 64-bit Solaris build? Does this latter work too?
I misread what you said. I thought that you were setting a restriction on the value of CC passed to make. Yes, I agree that the value set in the coreconf/*.mk files needs to be a single word, otherwise we'll clobber the useful flags. For the specific case that I cited, the restriction works but only because you have another mechanism to handle that specific case. If I wanted to do a CC='cc -o32' build on Irix (to get plugin compatibility with 4.x), then we'd be back to square one. Right now, it's more common to say CC='cc -xarch=v9' to get a 64-bit build as it is guaranteed to work across most packages. In theory, either method should work but I haven't tested the latter. The problem is that some cflags, like -xarch=v9, change the behavior of the compiler so much that you must make sure that all binaries are compiled with that flag. Usually, setting CC is the only way to keep that guarantee.
Chris Seawood wrote: > I misread what you said. I thought that you were setting > a restriction on the value of CC passed to make. Actually I want to set that restriction as well. I know it will be okay to pass a multi-word value of CC to make, at least I don't have a real-world counterexample, but I just feel that I need to think more about it. For your new example of CC='cc -o32' on IRIX, you can use the coreconf option USE_N32=, although coreconf won't do the right thing if the version of cc you are using generates n32 binaries by default. This can be fixed by modifying coreconf to use -o32 if USE_N32 is undefined or adding a new coreconf option USE_O32. My point is that coreconf usually has an option for each compiler flag that you want to specify in the value of CC. For example, coreconf has USE_64=1 for CC='cc -xarch=v9' and USE_N32= for CC='cc -o32'. I propose that we first proceed with the restriction that *both* the value of CC passed to make and the value set in the coreconf/*.mk files need to be a single word, and get your patch checked in. Then, I will think it over whether we can allow passing a multi-word value of CC to make. How does this sound?
> My point is that coreconf usually has an option for each > compiler flag that you want to specify in the value of CC. And when coreconf doesn't have an equivalent option, then we are blocked waiting for someone to find the time to: 1) verify that our problem is actually a coreconf problem and 2) deem it important enough to devote resources to fixing Historically, neither of those steps has typically been done in a timely manner. Why make the developers bother going through that timesink when we can just build flexibility into the system to allow multi-word CCs to be passed in? Coreconf loses nothing by allowing multi-word CCs to be passed in and developers gain the flexibility of being able to use their "non-standard" configurations without being unnecessarily blocked.
I haven't had time to work on this. Sigh. I am moving the target to NSS 4.0.
Target Milestone: 3.4 → 4.0
Is something happening to this?
I'd like to gently push for this as well. I'm building mozilla for the iPaq, and I have to cross-compile, because the iPaq doesn't have enough resources to compile natively. Being able to override CC and CCC would greatly simplify the task of cross-compiling security. I'd say that in general, this is a moderately important bug for getting Mozilla running on embedded devices.
Hm. Chris Seawood just pointed me at bug 104541. Is this a dup or otherwise deserving of abandonment?
No, this isn't a dupe of 104541. There are valid uses of overriding CC/CXX other than just for cross-compiling. And according to bug 104541#c2, just overriding CC/CXX isn't enough to get a proper cross-compilation env setup for NSS.
Blocks: 158385
*** Bug 166305 has been marked as a duplicate of this bug. ***
What do you think about the patch I had attached to bug 166305?
The patch from bug 93206 was more robust.
Yes. We should move that patch (attachment 48743 [details] [diff] [review]) to this bug.
Attached patch Add CXXFLAGS to coreconf (deleted) — Splinter Review
Attachment 48743 [details] [diff] applied to the current NSS_CLIENT_TAG pull.
Comment on attachment 98203 [details] [diff] [review] Add CXXFLAGS to coreconf Dauphin asked me to review the patch. It looks good to me, but I suggest that Wan-Teh should review as well, since I'm not really a NSS person.
Attachment #98203 - Flags: review+
Target Milestone: 4.0 → 3.7
Moved to target milestone 3.8 because the original NSS 3.7 release has been renamed 3.8.
Target Milestone: 3.7 → 3.8
Attachment #98203 - Flags: review+ → review?(wtc)
Attached patch Minimal patch for coreconf (obsolete) (deleted) — Splinter Review
This patch is the absolute minimum changes required for Mozilla to override the compilers used by NSS. Note the two simplifications: 1. It only allows the C compiler to be overridden. This is sufficient for NSS because NSS does not have any C++ code. 2. It requires that the value of CC be a pathname, with no command-line options. I understand that this may turn out to be a limitation, but I suggest that we check in this patch first because it is still a step forward. I will attach the corresponding PSM patch to bug 93206.
Attachment #114057 - Flags: review?(seawood)
Attached patch Minimal patch for coreconf, v2 (deleted) — Splinter Review
I found that GNU make's "basename" function does not do the same thing as C's "basename" function :-) The GNU make function we want is "notdir".
Attachment #114057 - Attachment is obsolete: true
Attachment #114057 - Flags: review?(seawood)
Attachment #114058 - Flags: review?(seawood)
Attachment #114058 - Flags: review?(seawood) → review+
Remove target milestone of 3.8, since these bugs didn't get into that release.
Target Milestone: 3.8 → ---
A form of this patch has been checked into the NSS tree some time ago. See http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/security/coreconf/ruleset.mk .
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: