Closed
Bug 794896
Opened 12 years ago
Closed 12 years ago
Make the NSS build system friendly to pymake
Categories
(NSS :: Build, defect)
NSS
Build
Tracking
(Not tracked)
RESOLVED
FIXED
3.14.1
People
(Reporter: glandium, Assigned: glandium)
References
Details
Attachments
(3 files)
(deleted),
patch
|
rrelyea
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
rrelyea
:
review+
|
Details | Diff | Splinter Review |
pymake tries to run commands itself, but for complex constructs, it delegates to an actual shell. So things like "cd $(dir); $(MAKE)" are run with a subshell instead of within pymake directly. On Windows, this is even more a problem because calling a subshell to call $(MAKE) means going from a win32 app to a msys app, and then back to a win32 app, and chances are the msys shell will have botched the environment in the middle (and it does, when building NSS as part of mozilla-central build) Also, using $(shell pwd) doesn't quite work with pymake because that returns a msys path, which pymake can't really deal with.
Assignee | ||
Comment 1•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #665380 -
Flags: review?(rrelyea)
Assignee | ||
Updated•12 years ago
|
Attachment #665380 -
Flags: review?(kaie)
Comment 2•12 years ago
|
||
In general I think the patch is good. I do a few questions: What happens if there are values for non-existed directories in $DIRS? This is currently (and purposefully) allowed in the current NSS build system. It looks like we don't get the expected 'skipping....' error message bob
Assignee | ||
Comment 3•12 years ago
|
||
(In reply to Robert Relyea from comment #2) > In general I think the patch is good. I do a few questions: > > What happens if there are values for non-existed directories in $DIRS? This > is currently (and purposefully) allowed in the current NSS build system. It > looks like we don't get the expected 'skipping....' error message It will break, but there's no reliable way to make that work without creating problems with pymake builds. FWIW, I haven't had build problems with this patch on try, but it's only for mozilla-central, not for a generic nss build.
Comment 4•12 years ago
|
||
OK, I think I want to talk to the other NSS team members about this. I'm not really comfortable with losing the semantic, on the other hand it's probably not super critical and I can probably be talked off the ledge on this. Is pymake something that's being used on the mozilla back end to improve build time? (My thinking here is we're pitting an old, favored semantic which I'm not sure how pervasive it's use is against the ability to use pymake. I think that answer will tip on which of these two are more important to the NSS team, so know the importance of pymake to mozilla would weigh heavily in this calculation). bob
Assignee | ||
Comment 5•12 years ago
|
||
> Is pymake something that's being used on the mozilla back end to improve build time?
For nss, it doesn't bring much currently, but it's a first step towards being able to do parallel builds (gnu make tends to lock when doing parallel builds on windows). The other advantage of using pymake is to avoid some of the headaches that come from running the nss build through gmake while all the rest is built with pymake.
Updated•12 years ago
|
Attachment #665380 -
Flags: review?(kaie)
Comment 6•12 years ago
|
||
Comment on attachment 665380 [details] [diff] [review] Make the NSS build system friendly to pymake r+ OK, I've pinged the other NSS developers and got nothing, and I've convinced myself the semantic change does more good than harm so I'm r+ this patch.
Attachment #665380 -
Flags: review?(rrelyea) → review+
Updated•12 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 8•12 years ago
|
||
(In reply to Ryan VanderMeulen from comment #7) > This needs checkin for upstream NSS, not m-c, correct? Indeed
Assignee | ||
Comment 9•12 years ago
|
||
Who can land this (if possible before 3.14 final, so that we can have it on m-c soon)
Comment 10•12 years ago
|
||
Sorry, it was too late for 3.14. Release candidates are done and we're waiting for final test feedback before declaring 3.14 RTM. We can check in once 3.14 RTM has been tagged. Setting target to 3.14.1
Target Milestone: --- → 3.14.1
Comment 11•12 years ago
|
||
Checking in coreconf/rules.mk; /cvsroot/mozilla/security/coreconf/rules.mk,v <-- rules.mk new revision: 1.85; previous revision: 1.84 done Checking in coreconf/ruleset.mk; /cvsroot/mozilla/security/coreconf/ruleset.mk,v <-- ruleset.mk new revision: 1.24; previous revision: 1.23 done Checking in nss/Makefile; /cvsroot/mozilla/security/nss/Makefile,v <-- Makefile new revision: 1.42; previous revision: 1.41 done
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
Keywords: checkin-needed
Comment 12•12 years ago
|
||
I would like to have one nightly build of Firefox with the NSS 3.14 RTM tag, before we land this change in mozilla-central. Based on Wan-Teh's recommendation in nss-dev, I will tag the NSS tree with a special tag (NSS_3_14_1_MOZILLA_CENTRAL_1) and import that tag into mozilla-central next week, after we've landed NSS 3.14 RTM in -central and -aurora.
Comment 14•12 years ago
|
||
Mike: this patch suggests a solution to the problem, but please feel free to use a different solution. Thanks. The SUBMAKE function is defined to take two arguments. $(1) is a makefile target and $(2) is a directory. $(2) may be empty. But in the rule for the $(DIRS) target (in rules.mk), we call SUBMAKE with only one argument, and that argument is a directory. It seems that we should call SUBMAKE with two arguments, as follows: ifdef DIRS $(DIRS):: $(call SUBMAKE,,$@) endif But the makefile rule for $(DIRS) is unused, as this MXR query shows: http://mxr.mozilla.org/security/search?string=$%28DIRS%29 No makefile target depends on $(DIRS). So I suggest we simply remove the makefile rule for $(DIRS), including its documentation in README. This patch also contains some code cleanup. 1. The '+' is not necessary if the command contains $(MAKE). See http://www.gnu.org/software/make/manual/html_node/Instead-of-Execution.html#Instead-of-Execution If you deliberately used '+' for clarity, that's fine by me. 2. Remove the empty string check for $(2) in the SUBMAKE function. I don't think we ever do a sub-make in the current directory using the SUBMAKE function. 3. Remove the leading space in the third argument to the foreach function call in LOOP_OVER_DIRS.
Attachment #674279 -
Flags: review?(mh+mozilla)
Comment 15•12 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #3) > (In reply to Robert Relyea from comment #2) > > What happens if there are values for non-existed directories in $DIRS? This > > is currently (and purposefully) allowed in the current NSS build system. It > > looks like we don't get the expected 'skipping....' error message > > It will break, but there's no reliable way to make that work without > creating problems with pymake builds. FWIW, I haven't had build problems > with this patch on try, but it's only for mozilla-central, not for a generic > nss build. I updated my local copy of mozilla-central with the NSS head (python client.py update_nss HEAD) and pushed to try (https://tbpl.mozilla.org/?tree=Try&rev=1c37fecf4d52). The build fails on Android, I think because of this patch: cd softoken; make -j1 export make[7]: Entering directory `/builds/slave/try-andrd/build/security/nss/lib/softoken' cd dummy; make -j1 export make: Entering an unknown directory make: Leaving an unknown directory make: *** dummy: No such file or directory. Stop.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 16•12 years ago
|
||
(In reply to Brian Smith (:bsmith) from comment #15) > cd softoken; make -j1 export > make[7]: Entering directory > `/builds/slave/try-andrd/build/security/nss/lib/softoken' > cd dummy; make -j1 export > make: Entering an unknown directory > make: Leaving an unknown directory > make: *** dummy: No such file or directory. Stop. The softoken (and dbm) makefiles contain this: ifdef NSS_DISABLE_DBM DIRS= dummy endif It makes sense because android is built with NSS_DISABLE_DBM on Android, according to configure.in: case "${target}" in *-android*|*-linuxandroid*) if test "$CPU_ARCH" = "arm" ; then USE_ARM_KUSER=1 fi NSS_DISABLE_DBM=1 ...
Assignee | ||
Updated•12 years ago
|
Attachment #674279 -
Flags: review?(mh+mozilla) → review+
Assignee | ||
Comment 18•12 years ago
|
||
Comment on attachment 674554 [details] [diff] [review] Fix NSS_DISABLE_DBM builds (fixes Android and B2G bustage) Review of attachment 674554 [details] [diff] [review]: ----------------------------------------------------------------- ::: mozilla/security/dbm/Makefile @@ +10,5 @@ > > include manifest.mn > > ifdef NSS_DISABLE_DBM > +DIRS= You might as well remove the ifdef completely.
Comment 19•12 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #18) > ::: mozilla/security/dbm/Makefile > @@ +10,5 @@ > > > > include manifest.mn > > > > ifdef NSS_DISABLE_DBM > > +DIRS= > > You might as well remove the ifdef completely. I think I cannot, because manifest.mn sets DIRS="src include" by default.
Comment 21•12 years ago
|
||
Comment on attachment 674554 [details] [diff] [review] Fix NSS_DISABLE_DBM builds (fixes Android and B2G bustage) Nit: please add a space before '='. This is not a Bourne shell script. Thanks.
Comment 22•12 years ago
|
||
Comment on attachment 674279 [details] [diff] [review] The SUBMAKE function takes two arguments Patch checked in on the NSS trunk (NSS 3.14.1). Checking in README; /cvsroot/mozilla/security/coreconf/README,v <-- README new revision: 1.5; previous revision: 1.4 done Checking in rules.mk; /cvsroot/mozilla/security/coreconf/rules.mk,v <-- rules.mk new revision: 1.86; previous revision: 1.85 done Checking in ruleset.mk; /cvsroot/mozilla/security/coreconf/ruleset.mk,v <-- ruleset.mk new revision: 1.25; previous revision: 1.24 done
Comment 24•12 years ago
|
||
Well, I'm not sure, maybe it wasn't this change, because this change is NSS, while the breakage actually is in NSPR, so maybe it's rather bug 757593 who caused the breakage.
Comment 25•12 years ago
|
||
Comment on attachment 674554 [details] [diff] [review] Fix NSS_DISABLE_DBM builds (fixes Android and B2G bustage) r+, Though this may need to be DIRS:= to override any previous setting of DIRS?
Attachment #674554 -
Flags: review?(rrelyea) → review+
Comment 26•12 years ago
|
||
Actually I guess not since the patch wouldn't have fixed the ANDROID builds.
Comment 27•12 years ago
|
||
Checking in dbm/Makefile; /cvsroot/mozilla/security/dbm/Makefile,v <-- Makefile new revision: 1.5; previous revision: 1.4 done Checking in lib/softoken/Makefile; /cvsroot/mozilla/security/nss/lib/softoken/Makefile,v <-- Makefile new revision: 1.9; previous revision: 1.8 done
Updated•12 years ago
|
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•