Make the NSS build system friendly to pymake

RESOLVED FIXED in 3.14.1

Status

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

trunk
3.14.1
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

Assignee

Description

7 years ago
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

Updated

7 years ago
Blocks: 486141
Assignee

Updated

7 years ago
Attachment #665380 - Flags: review?(rrelyea)
Assignee

Updated

7 years ago
Attachment #665380 - Flags: review?(kaie)

Comment 2

7 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

7 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

7 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

7 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

7 years ago
Attachment #665380 - Flags: review?(kaie)

Comment 6

7 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

7 years ago
Keywords: checkin-needed
This needs checkin for upstream NSS, not m-c, correct?
Assignee

Comment 8

7 years ago
(In reply to Ryan VanderMeulen from comment #7)
> This needs checkin for upstream NSS, not m-c, correct?

Indeed
Assignee

Comment 9

7 years ago
Who can land this (if possible before 3.14 final, so that we can have it on m-c soon)
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
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: 7 years ago
Resolution: --- → FIXED

Updated

7 years ago
Keywords: checkin-needed
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 13

7 years ago
Please use the tag name NSS_HEAD_2012mmdd.

Comment 14

7 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)
(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 → ---
(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

7 years ago
Attachment #674279 - Flags: review?(mh+mozilla) → review+
Assignee

Comment 18

7 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.
(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.
Assignee

Comment 20

7 years ago
Ah. good point.

Comment 21

7 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

7 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
I suspect this change broke the build on Sun OS machine buildnss04.
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

7 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

7 years ago
Actually I guess not since the patch wouldn't have fixed the ANDROID builds.
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
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.