Closed Bug 727406 Opened 12 years ago Closed 12 years ago

Port bug 645356 - Use pymake builtins

Categories

(MailNews Core :: Build Config, defect)

defect
Not set
normal

Tracking

(thunderbird-esr1013+ fixed)

RESOLVED FIXED
Thunderbird 17.0
Tracking Status
thunderbird-esr10 13+ fixed

People

(Reporter: standard8, Assigned: standard8)

References

Details

Attachments

(2 files)

Attached patch The fixSplinter Review
+++ This bug was initially created as a clone of Bug #645356 +++

pymake has its own built-in mkdir, rm, sleep and touch implementations. We should make use of them where we can, and remove any redundant/default parameters while we're at it.



Whilst I know we're generally not porting much from m-c, I want this bug as I'm currently trying (in bug 668869) to make Thunderbird's important files in mail/app/ as close as possible to Firefox's in terms of diffs etc. Hence I need the make variables introduced by this patch to make it happen.

Note: I've ported the <app>/installer parts, but I purposely didn't port the <app>/app parts as they are in flux at the moment (and I'm addressing mail/app in bug 668869).
Attachment #597357 - Flags: review?(bugspam.Callek)
Attachment #597357 - Flags: review?(bugspam.Callek) → review+
Assignee: nobody → mbanner
Checked in: http://hg.mozilla.org/comm-central/rev/003487521298
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 13.0
Comment on attachment 597357 [details] [diff] [review]
The fix

Review of attachment 597357 [details] [diff] [review]:
-----------------------------------------------------------------

::: config/rules.mk
@@ -2097,4 @@
>  	$(CHECK_FROZEN_VARIABLES)
>  
>  default all::
> -	if test -d $(DIST)/bin ; then touch $(DIST)/bin/.purgecaches ; fi

ugh missed that this change (and possibly some other ones that are called from shell) break:

c:\t1\hg\comm-central\config\rules.mk:2082:0$ if test -d ../../../mozilla/dist/bin ; then %pymake.builtins touch ../../../mozilla/dist/bin/.purgecaches ; fi
/usr/bin/sh: line 0: fg: no job control
c:\t1\hg\comm-central\config\rules.mk:2082:0: command 'if test -d ../../../mozilla/dist/bin ; then %pymake.builtins touch ../../../mozilla/dist/bin/.purgecaches ; fi' failed, return code 1
c:\t1\hg\objdir-sm\suite\locales\Makefile:180:0: command 'c:/DEV/mozilla-build/python/python.exe c:/t1/hg/comm-central/mozilla/build/pymake/pymake/../make.py -C ../../editor/ui/locales AB_CD=en-US XPI_NAME=locale-en-US BOTH_MANIFESTS=1' failed, return code 2
c:\t1\hg\objdir-sm\suite\installer\Makefile:165:0: command 'c:/DEV/mozilla-build/python/python.exe c:/t1/hg/comm-central/mozilla/build/pymake/pymake/../make.py -C ../../suite/locales langpack' failed, return code 2
c:\t1\hg\comm-central\config\rules.mk:676:0: command 'c:/DEV/mozilla-build/python/python.exe c:/t1/hg/comm-central/mozilla/build/pymake/pymake/../make.py libs' failed, return code 2
c:\t1\hg\comm-central\suite\build.mk:79:0: command 'c:/DEV/mozilla-build/python/python.exe c:/t1/hg/comm-central/mozilla/build/pymake/pymake/../make.py -C suite/installer' failed, return code 2

Please backout and submit a new patch
This can't be backed out without backing out the dependent patch which I don't want to do.

I don't have time now, but I can back out that one line change from the patch tomorrow morning my time.
(In reply to Mark Banner (:standard8) from comment #3)
> This can't be backed out without backing out the dependent patch which I
> don't want to do.
> 
> I don't have time now, but I can back out that one line change from the
> patch tomorrow morning my time.

I explicitly asked for a backout and a new patch, since you NEED to check all the cases of line continuations for breakage here, if it tries to run more than one command in a single invocation it will invoke the shell and BREAK in pymake.

That is the whole problem here, though I do find it annoying that "I don't want to" is good enough for leaving the tree broken for normal developers for a good amount of time.
(In reply to Justin Wood (:Callek) from comment #4)
> (In reply to Mark Banner (:standard8) from comment #3)
> > This can't be backed out without backing out the dependent patch which I
> > don't want to do.
> > 
> > I don't have time now, but I can back out that one line change from the
> > patch tomorrow morning my time.
> 
> I explicitly asked for a backout and a new patch, since you NEED to check
> all the cases of line continuations for breakage here, if it tries to run
> more than one command in a single invocation it will invoke the shell and
> BREAK in pymake.

Like I said, I didn't have time to look at it last night, and didn't want to back out both bugs (I never said I wouldn't).

Having now looked at it afresh, I've backed out just the rules.mk change. This would seem to satisfy the not breaking pymake requirement and being able to leave the additional bug in without too many extra changes.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Mark Banner (:standard8) from comment #5)
> (In reply to Justin Wood (:Callek) from comment #4)
> Having now looked at it afresh, I've backed out just the rules.mk change.
> This would seem to satisfy the not breaking pymake requirement and being
> able to leave the additional bug in without too many extra changes.

The backout was http://hg.mozilla.org/comm-central/rev/271f2a50cfbb
Comment on attachment 597357 [details] [diff] [review]
The fix

[Triage Comment]
We need this and the backout on ESR to fix build bustage with the new automation, so a=me for landing.
Attachment #597357 - Flags: approval-comm-esr10+
Attached patch Re-do rules.mkSplinter Review
This ports just the rules.mk changes from bug 645356. This time, I just replaced the same instances in c-c's file as were replaced in the m-c file, rather than replacing all the instances.

Try server builds will appear here:

https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=7dcfa0f4f25f
Attachment #652563 - Flags: review?(bugspam.Callek)
Comment on attachment 652563 [details] [diff] [review]
Re-do rules.mk

Review of attachment 652563 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the one nit/typo fixed.

::: config/rules.mk
@@ +1019,4 @@
>  endif
>  
>  $(filter %.$(LIB_SUFFIX),$(LIBRARY)): $(OBJS) $(LOBJS) $(EXTRA_DEPS) $(GLOBAL_DEPS)
> +	$(EM) $(LIBRARY)

Umm, this one looks wrong. Should be $(RM) as well.
Attachment #652563 - Flags: review?(bugspam.Callek) → review+
Checked in with comment addressed: https://hg.mozilla.org/comm-central/rev/29ac9144a9fc
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Target Milestone: Thunderbird 13.0 → Thunderbird 17.0
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: