Closed Bug 546642 Opened 10 years ago Closed 10 years ago

Cleanup during make distclean

Categories

(MailNews Core :: Build Config, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch Patch (obsolete) — Splinter Review
+++ This bug was initially created as a clone of Bug #541770 +++

A bunch of files are not cleaned up from the main directory when doing make distclean.
Keywords: checkin-needed
Attachment #427325 - Attachment is patch: true
Attachment #427325 - Attachment mime type: application/octet-stream → text/plain
Attachment #427325 - Flags: review?(kairo)
Attachment #427325 - Flags: review?(kairo) → review+
Attached patch Patch v2 (obsolete) — Splinter Review
Some more cleanup.
Attachment #427325 - Attachment is obsolete: true
Attachment #427338 - Flags: review?(kairo)
Attachment #427338 - Flags: review?(kairo) → review+
Comment on attachment 427338 [details] [diff] [review]
Patch v2

>diff --git a/Makefile.in b/Makefile.in
>+
>+distclean::
>+	cat unallmakefiles | $(XARGS) rm -f
>+	rm -f unallmakefiles $(DIST_GARBAGE)
>+
>+DIST_GARBAGE = config.cache config.log config.status \
>+   config/autoconf.mk \
>+   unallmakefiles comm-config.h \
>+   $(topsrcdir)/.mozconfig.mk $(topsrcdir)/.mozconfig.out

This diff can't be right: similar code already exist since bug 446690.
(Please fully fix bug 541767 first.)
Attachment #427338 - Flags: review-
Assignee: nobody → mh+mozilla
Status: NEW → ASSIGNED
Flags: in-testsuite-
Product: SeaMonkey → MailNews Core
QA Contact: build-config → build-config
OS: Linux → All
Hardware: x86 → All
The diff is right, except it is against 1.9.1
(In reply to comment #3)
> The diff is right, except it is against 1.9.1

Wouahou! So you (silently) cloned a m-c /js/src patch to create a c-1.9.1 patch?
What's wrong with porting from c-c (i.e. the bug that fixed it there) and being explicit about your target?
As in: I'm happy you care, but why create a difference between c-c and c-1.9.1?
I'm sending patches against what I have and at the moment it is 1.9.1. It turns out most of the time, they apply as is on trunk, but not in the current case. (and the m-c js/src patch was also against 1.9.1 originally).

Also note that the change in Makefile.in on c-c occurred less than a month ago...

Anyways, do you realize that instead of arguing both of us could have spent this time updating the patch ?
Attached patch Patch for c-cSplinter Review
Here you are
Attachment #427338 - Attachment is obsolete: true
Attachment #427731 - Flags: review?(sgautherie.bz)
(In reply to comment #5)

> I'm sending patches against what I have and at the moment it is 1.9.1. It turns
> out most of the time, they apply as is on trunk, but not in the current case.

Good for you.
But you're explicitly supposed *not* to do that, especially without _warning_ people.

> Also note that the change in Makefile.in on c-c occurred less than a month
> ago...

... which is much older than your 2-days old patch!

> Anyways, do you realize that instead of arguing both of us could have spent
> this time updating the patch ?

You, probably.
But not me, as I'm yet trying to figure out what you're doing:
for example, it looks like your new patch (context) here applies on top of your additional patch in bug 541767, though (it's fine but) you didn't say it.

Again, I appreciate you're doing patches, but please either do it by the rules or explain what you're doing differently: it really helps the other people involved.
Attachment #427731 - Flags: review?(sgautherie.bz) → review?(kairo)
> But you're explicitly supposed *not* to do that, especially without _warning_
> people.

I usually do. Sorry to have been sloppy once in a while.

> for example, it looks like your new patch (context) here applies on top of your
> additional patch in bug 541767, though (it's fine but) you didn't say it.

But you asked for it. cf. comment 2.
Attachment #427731 - Flags: review?(kairo) → review+
(In reply to comment #8)

> I usually do. Sorry to have been sloppy once in a while.

np. (Just don't react as if all was as expected when I wonder what is going on.)

> But you asked for it. cf. comment 2.

Yes and it's perfectly fine to have done it (that way):
I was simply saying that you are expected to tell the (context) dependency (especially from different bugs) when you attach a patch.

***

I was going (again) to check this in, but I still wonder:
for example, I see $(MOZ_APP_NAME).1 in no other app.

Ftr, could you write (in the bug) a (1+ line) explanation about why you're adding/removing each file? Thanks.
(In reply to comment #9)
> Ftr, could you write (in the bug) a (1+ line) explanation about why you're
> adding/removing each file? Thanks.

Not necessary, I checked what all file changes are for. SeaMonkey has that .1 used.
Keywords: checkin-needed
http://hg.mozilla.org/comm-central/rev/48a4baab97ba
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.