Closed Bug 542222 Opened 11 years ago Closed 11 years ago

Reduce recursion in makefiles

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: Mitch, Assigned: Mitch)

References

Details

Attachments

(3 files, 1 obsolete file)

Follow-up to bug 461444.
Attachment #423528 - Flags: review?(ted.mielczarek)
It looks like the renaming of the files under extensions didn't retain history... perhaps not using the standard hg push without extensions (e.g. qimport, or whatever) will retain the history properly. I would really appreciate keeping the history on these files.
Yeah, just had this discussion with Mossop on IRC. Mitch's patch had the renames, and I qimported it directly, and at one point the renames were correct locally, but perhaps a rebase or something lost that data. :-(
Attachment #423528 - Flags: review?(dtownsend)
Attachment #423528 - Flags: review?(dtownsend) → review?(robert.bugzilla)
Comment on attachment 423528 [details] [diff] [review]
toolkit/mozapps/update (checked in)

>diff --git a/toolkit/mozapps/installer/wince/Makefile.in b/toolkit/mozapps/installer/wince/Makefile.in
>--- a/toolkit/mozapps/installer/wince/Makefile.in
>+++ b/toolkit/mozapps/installer/wince/Makefile.in
>@@ -40,27 +40,27 @@ topsrcdir = @top_srcdir@
> srcdir    = @srcdir@
> VPATH     = @srcdir@
> 
> include $(DEPTH)/config/autoconf.mk
> 
> export NO_SHUNT = 1
> USE_STATIC_LIBS = 1
> 
>-PROGRAM   = xulrunner-stub-installer.sfx
>+PROGRAM = xulrunner-stub-installer.sfx
> 
>-REQUIRES  = lib7z
>+REQUIRES = lib7z

Detabification is fine, but if you're going to go re-indenting all of these lines, you should make sure it's ok with Rob.

Looks fine otherwise.
Attachment #423528 - Flags: review?(ted.mielczarek) → review+
Comment on attachment 423528 [details] [diff] [review]
toolkit/mozapps/update (checked in)

I'm fine with the detabification... it is just leftover from what used to be the standard.
Attachment #423528 - Flags: review?(robert.bugzilla) → review+
Also, please verify that the history isn't lost after checkin and fix if it isn't.
I can't compile the bsdiff tool (other-licenses/bsdiff, required for making partial update patches) since this patch landed. The preceding changeset (37928=>cf63181a4e51) works fine.

Here's the error output: http://pastebin.mozilla.org/701304
Blocks: 535369
Fix for other-licenses/bsdiff/Makefile.in. Comment-only fixes in xpcom/glue and toolkit/content/license.html.
Attachment #425613 - Flags: review?(ted.mielczarek)
Comment on attachment 425613 [details] [diff] [review]
toolkit/mozapps/update fix (checked in)

Looks good and thanks... I should have caught that.

Sorry about that coop. I'll get this landed soon
Attachment #425613 - Flags: review?(ted.mielczarek) → review+
Comment on attachment 425613 [details] [diff] [review]
toolkit/mozapps/update fix (checked in)

Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/2d873df39b6a
Attachment #425613 - Attachment description: toolkit/mozapps/update fix → toolkit/mozapps/update fix (checked in)
Attachment #423528 - Attachment description: toolkit/mozapps/update → toolkit/mozapps/update (checked in)
(In reply to comment #10)
> Sorry about that coop. I'll get this landed soon

Fix WFM too. Thanks.
Depends on: 545257
Attached patch netwerk (obsolete) — Splinter Review
This patch passes TryServer.
Attachment #444521 - Flags: review?(cbiesinger)
Attachment #444521 - Flags: review?(cbiesinger) → review+
Comment on attachment 444521 [details] [diff] [review]
netwerk

For HTTP at least, you forgot to copy gagan's name from the contributor line, and for cookie you forgot to copy IBM. Looks good otherwise, thanks.
Attached patch netwerk (v2)Splinter Review
Attachment #444521 - Attachment is obsolete: true
Attachment #448708 - Flags: review?(cbiesinger)
Comment on attachment 448708 [details] [diff] [review]
netwerk (v2)

I'm not going to read this all again. assuming the only change is the added contributor lines, r=biesi
Attachment #448708 - Flags: review?(cbiesinger) → review+
Keywords: checkin-needed
So, uhm, the landing of this in http://hg.mozilla.org/mozilla-central/rev/d8dc49d5bd60 seems to have clobbered Hg history for everything touched in /netwerk. :-( Same as happened before in comment 2.

Is there a way to fix this? That sucks.
(Ugh, I see the same thing having happened to files I "know" I hg renamed in previous commits... Is something broken with Hg/mq wrt to renames?)
(Ah-ha, Gavin pointed out that "hg log -f foo.cpp" shows that history is still there; even though hgweb and "hg log foo.cpp" cut it off before the rename. Bug on file is http://mercurial.selenic.com/bts/issue1576, which means the problem noted in comment 2 is the same issue.)

Sorry for the bugspam. :-/
Um, I see no evidence that this patch was actually landed as file renames; it looks like what got landed was a bunch of adds and removes, which is a really serious mistake.
Well after a brief discussion with David on IRC, it appears I had an old[er] HG version, with bugs. I rebased before I pushed, and rebasing in my HG version breaks renames :/

Anyway, the patch with broken renames was pushed as http://hg.mozilla.org/mozilla-central/rev/d8dc49d5bd60
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
I've relanded the patch with correct renames as <http://hg.mozilla.org/mozilla-central/rev/2d90590dabe6>.  However, until hg annotate is fixed, the life of people who want to annotate netwerk source files is harder than necessary.
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.