794896 - Make the NSS build system friendly to pymake

Status: RESOLVED FIXED

(NSS :: Build, defect)

Description

[Mike Hommey [:glandium]]

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.

Comment 1

[Mike Hommey [:glandium]]

Attachment: Make the NSS build system friendly to pymake

Comment 2

[Robert Relyea]

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

Comment 3

[Mike Hommey [:glandium]]

(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

[Robert Relyea]

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

Comment 5

[Mike Hommey [:glandium]]

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

Comment 6

[Robert Relyea]

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.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

This needs checkin for upstream NSS, not m-c, correct?

Comment 8

[Mike Hommey [:glandium]]

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

Indeed

Comment 9

[Mike Hommey [:glandium]]

Who can land this (if possible before 3.14 final, so that we can have it on m-c soon)

Comment 10

[Kai Engert (:KaiE:)]

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

Comment 11

[Kai Engert (:KaiE:)]

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

Comment 12

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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

[Wan-Teh Chang]

Please use the tag name NSS_HEAD_2012mmdd.

Comment 14

[Wan-Teh Chang]

Attachment: The SUBMAKE function takes two arguments

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.

Comment 15

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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

Comment 16

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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

Comment 17

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Fix NSS_DISABLE_DBM builds (fixes Android and B2G bustage)

Comment 18

[Mike Hommey [:glandium]]

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

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(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 20

[Mike Hommey [:glandium]]

Ah. good point.

Comment 21

[Wan-Teh Chang]

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

[Wan-Teh Chang]

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 23

[Kai Engert (:KaiE:)]

I suspect this change broke the build on Sun OS machine buildnss04.

Comment 24

[Kai Engert (:KaiE:)]

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

[Robert Relyea]

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?

Comment 26

[Robert Relyea]

Actually I guess not since the patch wouldn't have fixed the ANDROID builds.

Comment 27

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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