Closed
Bug 93206
Opened 23 years ago
Closed 19 years ago
PSM doesn't pass CC variable to NSS make
Categories
(Core Graveyard :: Security: UI, defect, P2)
Core Graveyard
Security: UI
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla1.8beta3
People
(Reporter: bryner, Assigned: cls)
References
Details
Attachments
(1 file, 6 obsolete files)
1.45 KB,
patch
|
cls
:
review+
julien.pierre
:
superreview+
shaver
:
approval1.8b3+
|
Details | Diff | Splinter Review |
When security/manager/Makefile.in builds NSS, it should pass Mozilla's CC variable as a parameter. This will ensure that the same compiler is used to build NSS as is used for the rest of the Mozilla build. (We allow specifying an alternate compiler by setting CC before running configure).
Comment 1•23 years ago
|
||
P3 ->ddrinan t->2.1
Assignee: ssaux → ddrinan
Priority: -- → P3
Target Milestone: --- → 2.1
Reporter | ||
Comment 3•23 years ago
|
||
r=bryner
Comment 5•23 years ago
|
||
Move to future. Won't have time to fix these for 2.1
Target Milestone: 2.1 → Future
Comment 7•23 years ago
|
||
Allowing CC and CCC to be overriden on the gmake command line was not a goal of coreconf when it was designed. So we would first need to retrofit coreconf so that CC and CCC can be overriden safely. Makefile fragments in coreconf like the following would need to be changed. AIX.mk: ifeq ($(CC),xlC_r) HP-UXB.11.mk: CCC = /opt/aCC/bin/aCC -ext OS2.mk: CC = icc -q -DXP_OS2 -DOS2=4 -N10 SunOS5.mk: CCC = g++ CCC += -Wall -Wno-format ruleset.mk: ifneq ($(DEFAULT_COMPILER), $(CC))
Taking the bug as I spent a few hrs this evening wondering why the upgraded nebiros couldn't build NSS so I'm a bit motivated to get this fixed now. So, this patch is going to look wacky because: 1) previously, there was no CXXFLAGS. 2) CCC was being set with extra (presumably c++ only) flags 3) C++ files were being compiled with CCC & CFLAGS. Now, 1) CCC is just the compiler 2) the extra flags are being added to OS_CXXFLAGS 3) CXXFLAGS == CFLAGS + OS_CXXFLAGS 4) c++ files are being compiled with CCC & CXXFLAGS
Assignee: ddrinan → cls
Priority: P3 → P2
Target Milestone: Future → ---
Assignee | ||
Comment 10•23 years ago
|
||
Attachment #44679 -
Attachment is obsolete: true
Attachment #44702 -
Attachment is obsolete: true
Comment 11•23 years ago
|
||
Chris, thanks for taking this in. Adding patch, review. We need to assign a target for this. If this is absolutely necessary for the 0.9.4 branch, then it should be targetted 2.1 otherwise Future. Let me know.
Comment 12•23 years ago
|
||
Chris's patch to introduce CXXFLAGS to coreconf is the right solution. I still need to review it again carefully to make sure that all the necessary changes have been made. For example, in coreconf/OS2.mk, the line: CC = icc -q -DXP_OS2 -DOS2=4 -N10 should also be broken into two lines: CC = icc OS_CFLAGS += icc -q -DXP_OS2 -DOS2=4 -N10 In coreconf/ruleset.mk, the line: COMPILER_TAG = _$(CC) should now read: COMPILER_TAG = _$(firstword $(basename $(CC))) Also, the expression $(firstword $(basename $(DEFAULT_COMPILER))) may not be necessary because I believe DEFAULT_COMPILER is always a simple command name.
Assignee | ||
Comment 14•23 years ago
|
||
The NSS_CLIENT_TAG tagged version of coreconf/OS2.mk (v1.7) doesn't have the 'CC=icc ...' line. Here's the updated version of coreconf/ruleset.mk (the rest of the patch is the same): Index: mozilla/security/coreconf/ruleset.mk =================================================================== RCS file: /cvsroot/mozilla/security/coreconf/ruleset.mk,v retrieving revision 1.12 diff -u -r1.12 ruleset.mk --- ruleset.mk 2001/06/02 06:03:14 1.12 +++ ruleset.mk 2001/09/19 07:43:58 @@ -111,14 +111,14 @@ # ifndef COMPILER_TAG -ifneq ($(DEFAULT_COMPILER), $(CC)) +ifneq ($(DEFAULT_COMPILER), $(firstword $(basename $(CC)))) # # Temporary define for the Client; to be removed when binary release is used # ifdef MOZILLA_CLIENT COMPILER_TAG = else - COMPILER_TAG = _$(CC) + COMPILER_TAG = _$(firstword $(basename $(CC))) endif else COMPILER_TAG =
Updated•23 years ago
|
QA Contact: ckritzer → junruh
Comment 15•23 years ago
|
||
I created an NSS build system RFE (bug 107976) for the coreconf work.
Depends on: 107976
Attachment #48743 -
Attachment is obsolete: true
Comment 16•22 years ago
|
||
Comment on attachment 48745 [details] [diff] [review] Set CCC not CXX r=kaie Is it necessary to land bug 107976 and bug 93206 at the same time?
Attachment #48745 -
Flags: review+
Comment 17•22 years ago
|
||
The PSM patch requires the patch to NSS (actually coreconf) in order to work correctly for all platforms.
Comment 18•22 years ago
|
||
*** Bug 170266 has been marked as a duplicate of this bug. ***
Updated•22 years ago
|
Target Milestone: 2.2 → 2.4
Version: 2.0 → 2.4
Comment 19•22 years ago
|
||
This patch should be used in conjunction with the coreconf minimal patch (attachment 114058 [details] [diff] [review]) in bug 107976. Note the two simplifications: 1. Only CC is overriden. CCC (the C++ compiler) is not overridden, which is fine because NSS has no C++ code. 2. I'm taking the first word of $(CC) and passing it to NSS. The reason is that I am not sure if I can allow passing arbitrary compiler flags to NSS. I propose that we check in this patch first with this limitation and see how things go.
Updated•22 years ago
|
Attachment #114059 -
Flags: review?(kaie)
Updated•22 years ago
|
Attachment #114059 -
Flags: superreview?(seawood)
Comment 20•22 years ago
|
||
Comment on attachment 114059 [details] [diff] [review] Minimal patch: only override CC I still think the single word restriction is unnecessary. It's also fairly easy, if highly annoying, to get around that restriction by using a wrapper script. If you're going to accept an arbitrary $CC, I don't see the how accepting arbitrary args to that $CC is any worse.
Attachment #114059 -
Flags: superreview?(seawood) → superreview-
Comment 21•22 years ago
|
||
Some code in NSS must be compiled with certain compiler flags so that we only use certain instruction sets. If we accept arbitrary args to $(CC), we will lose that control of the instruction set used. We will not be able to honor all the build customizations. The NSS coreconf build system and the original NSPR build system ares designed to ensure that everyone builds our software the same way as we do, using the compiler flags we have tested. We recognize that it is common for people to have multiple versions of a compiler installed and the ability to specify a particular version of a compiler with its full pathname is useful. This is why we want to allow people to do this. I know you don't like the single word restriction. Please look at it this way: this patch, with its single word restriction, is still a step forward and better than what we have now. So I suggest that we check it in first and see if the single word restriction is really a problem.
Comment 22•22 years ago
|
||
> Some code in NSS must be compiled with certain compiler flags > so that we only use certain instruction sets. If we accept > arbitrary args to $(CC), we will lose that control of the > instruction set used. If you allow the user to set CC, then you've already lost that control. cat >/bin/annoy-nss-cc<<EOF #!/bin/sh exec gcc -mpentium4 $* EOF chmod 755 /bin/annoy-nss-cc env CC=annoy-nss-cc CXX='g++ -mpentium4' ../mozilla/configure It's already generally accepted that we cannot support every arbitrary compiler flag given to configure via --enable-optimize, CFLAGS/CXXFLAGS or CC/CXX. We routinely have to tell people to not use -fomit-frame-pointer, a common gcc optimization, as it kills xptcall which uses the frame pointer. However, we do not specifically prevent them from using *any* arbitrary flag because a handful of them may cause problems. If the user uses flags other than the default ones in the build system, then it's up to them to prove that problems caused by those flags aren't compiler issues. > The NSS coreconf build system and the original NSPR build > system ares designed to ensure that everyone builds our software > the same way as we do, using the compiler flags we have tested. If someone does a default NSS build, then they will build with those defaults. However, anyone could set CC on the commandline and add additional compiler flags to the build. You haven't protected against setting CC (or CFLAGS for that matter) in the environment in the NSS Makefiles. And as I pointed out earlier, the proposed change does not prevent me from working around that restriction though the workaround is inconvenient IMO. Given that the restriction is ineffective for its intended purpose, I don't see the point in adding the restriction. And I believe my points at http://bugzilla.mozilla.org/show_bug.cgi?id=107976#c6 are still valid.
Comment 23•22 years ago
|
||
Attachment #48745 -
Attachment is obsolete: true
Attachment #114059 -
Attachment is obsolete: true
Comment 24•22 years ago
|
||
Comment on attachment 114059 [details] [diff] [review] Minimal patch: only override CC Chris, I fully understand what you said. Checking in the patch I proposed would still be a *strict* improvement over what we have right now, and my patch does not preclude checking in your patch (removing the 'firstword' function) in the future. We should check in portions of your patch that everyone thinks are safe first, otherwise we are not making any progress on this bug.
Updated•22 years ago
|
Attachment #114059 -
Flags: review?(kaie)
Updated•21 years ago
|
QA Contact: junruh → bmartin
Comment 25•21 years ago
|
||
*** Bug 202186 has been marked as a duplicate of this bug. ***
Comment 26•21 years ago
|
||
Marking as OS-neutral, as this is not Linux-specific.
OS: Linux → All
Version: 2.4 → unspecified
Comment 27•21 years ago
|
||
*** Bug 219728 has been marked as a duplicate of this bug. ***
Comment 28•21 years ago
|
||
Attachment #114194 -
Attachment is obsolete: true
Comment 29•20 years ago
|
||
Comment on attachment 131805 [details] [diff] [review] Only set CC, with corresponding NSS patch cls, please review this patch. It's too bad that the COMPILER_TAG variable will have the wrong value if CC is "ccache gcc". But I can't come up with a good DEFAULT_COMPILER test that works in this case. One possible solution is to handle programs like ccache as a special case and use a new make variable like CC_FRONTEND for it.
Attachment #131805 -
Flags: review?(cls)
Attachment #131805 -
Flags: review?(cls) → review+
Comment 30•20 years ago
|
||
Comment on attachment 131805 [details] [diff] [review] Only set CC, with corresponding NSS patch Julien, please review the NSS change in this patch. I'd like to check in the NSS change on the tip and the NSS_3_9_BRANCH. This change (calling the firstword function on $(CC)) is a no-op for our own NSS builds because coreconf always sets CC to a value with only one word. However, the PSM change in this patch will override the value of CC in coreconf, so coreconf needs to protect against a value of CC with more than one word (e.g., "ccache gcc". see bug 250153). Note that with my proposed NSS change, if CC is "ccache gcc", COMPILER_TAG will be "ccache", which is wrong. A better long-term fix is probably to delete all the coreconf code related to DEFAULT_COMPILER and the automatic setting of COMPILER_TAG for non-default compilers. In other words, we would set COMPILER_TAG manually. Let me know if you'd like me to do this instead.
Attachment #131805 -
Flags: superreview?(julien.pierre.bugs)
Updated•20 years ago
|
Attachment #131805 -
Flags: superreview?(julien.pierre.bugs) → superreview+
Comment 31•20 years ago
|
||
Comment on attachment 131805 [details] [diff] [review] Only set CC, with corresponding NSS patch I checked in the NSS coreconf change in this patch on the NSS trunk (NSS 3.10). We still need to get the NSS coreconf change into NSS_CLIENT_TAG and check in the PSM change in this patch.
Attachment #131805 -
Flags: approval1.8b3?
Assignee | ||
Comment 32•19 years ago
|
||
*** Bug 250153 has been marked as a duplicate of this bug. ***
Comment on attachment 131805 [details] [diff] [review] Only set CC, with corresponding NSS patch a=shaver
Attachment #131805 -
Flags: approval1.8b3? → approval1.8b3+
Assignee | ||
Comment 34•19 years ago
|
||
The patch has been checked in.
Status: NEW → RESOLVED
Closed: 19 years ago
Hardware: PC → All
Resolution: --- → FIXED
Target Milestone: psm2.4 → mozilla1.8beta3
Version: Other Branch → Trunk
Updated•8 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•