Port bug 645356 - Use pymake builtins

RESOLVED FIXED in Thunderbird 17.0

Status

MailNews Core
Build Config
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

Trunk
Thunderbird 17.0
Dependency tree / graph
Bug Flags:
in-testsuite -

Thunderbird Tracking Flags

(thunderbird-esr1013+ fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
Created attachment 597357 [details] [diff] [review]
The fix

+++ 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)

Updated

5 years ago
Attachment #597357 - Flags: review?(bugspam.Callek) → review+
(Assignee)

Updated

5 years ago
Assignee: nobody → mbanner
(Assignee)

Comment 1

5 years ago
Checked in: http://hg.mozilla.org/comm-central/rev/003487521298
Status: NEW → RESOLVED
Last Resolved: 5 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
(Assignee)

Comment 3

5 years ago
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.
(Assignee)

Comment 5

5 years ago
(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 → ---
(Assignee)

Updated

5 years ago
Blocks: 760353
(Assignee)

Comment 6

5 years ago
(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
(Assignee)

Comment 7

5 years ago
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+
(Assignee)

Comment 8

5 years ago
Created attachment 652563 [details] [diff] [review]
Re-do rules.mk

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 9

5 years ago
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+
(Assignee)

Comment 10

5 years ago
Checked in with comment addressed: https://hg.mozilla.org/comm-central/rev/29ac9144a9fc
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: Thunderbird 13.0 → Thunderbird 17.0
(Assignee)

Updated

5 years ago
status-thunderbird-esr10: --- → fixed
tracking-thunderbird-esr10: --- → 13+
You need to log in before you can comment on or make changes to this bug.