Various problems with how config/my{config,rules}.mk are handled

RESOLVED FIXED in mozilla1.9.3a2

Status

Firefox Build System
General
RESOLVED FIXED
9 years ago
3 months ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

({dataloss})

Trunk
mozilla1.9.3a2
dataloss

Firefox Tracking Flags

(status1.9.2 ?)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

9 years ago
Created attachment 423207 [details] [diff] [review]
Patch

(I think this is the same on trunk)

I found out about this issue because config/myconfig.mk and config/myrules.mk are not removed from js/src, as well as some other files that are going to be subject of another bug report.

It appears that config/myconfig.mk and config/myrules.mk files can be used by the person who builds the tree to add custom config and rules. But when these files don't pre-exist, they are created, to speed up gmake a bit.

In js/src, it just stops here, so that after make clean or make distclean, empty files may be left over.

In the root directory, though, it is different, and they are part of the distclean wipe.

This means that custom rules and config are *lost* if they ever existed. If the mozconfig is kept after make distclean, I don't see why these ones shouldn't be kept as well.

The attached patch makes distclean not remove these files, and instead only include config/myconfig.mk and config/myrules.mk when they exist.
(Assignee)

Updated

9 years ago
Attachment #423207 - Attachment is patch: true
Attachment #423207 - Attachment mime type: application/octet-stream → text/plain
Attachment #423207 - Flags: review?(benjamin)

Updated

8 years ago
Attachment #423207 - Flags: review?(benjamin) → review?(ted.mielczarek)
Comment on attachment 423207 [details] [diff] [review]
Patch

I wouldn't bother with the "+ifneq (,$(wildcard $(MY_CONFIG)))" bit, that's why "-include" exists, so you can include a file but not error if it doesn't exist. I can't imagine gmake is going to be any faster at evaluating that wildcard and then doing the include than it will be just doing the include for a nonexistent file.

The rest of the patch looks fine.
Attachment #423207 - Flags: review?(ted.mielczarek) → review-
(Assignee)

Comment 2

8 years ago
The reason why I did this is because it would actually make it (slightly) faster: it won't try to create the file (i.e. finding a rule for it), which -include will end up trying to do.
I'm sure there's not an actual measurable difference between those. Feel free to prove otherwise, but without good reason, might as well keep it simple.
(Assignee)

Comment 4

8 years ago
My only rationale for trying to make it (slightly) faster is the comment justifying the creation of the myconfig.mk and myrules.mk files by the make rules: "This speeds up gmake's processing if these files don't exist."

I haven't checked how old this comment was, though.
(Assignee)

Comment 5

8 years ago
Created attachment 425223 [details] [diff] [review]
Patch v2
[Checkin: Comment 7]

Anyways, here is a version without the wildcard stuff. Feel free to use whichever you prefer.
Attachment #425223 - Flags: review?(ted.mielczarek)
Attachment #425223 - Flags: review?(ted.mielczarek) → review+
Assignee: nobody → mh+mozilla
Status: NEW → ASSIGNED
Keywords: checkin-needed
(Assignee)

Comment 6

8 years ago
Created attachment 427324 [details] [diff] [review]
Patch for comm-central
[Checkin: Comment 8]
Attachment #427324 - Flags: review?(kairo)

Updated

8 years ago
Attachment #427324 - Flags: review?(kairo) → review+
Pushed c-c patch as: http://hg.mozilla.org/comm-central/rev/9d7af06010f9
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
status1.9.2: --- → ?
Flags: in-testsuite-
Keywords: dataloss
OS: Linux → All
Hardware: x86 → All
Target Milestone: --- → mozilla1.9.3a2
Version: 1.9.1 Branch → Trunk
Comment on attachment 427324 [details] [diff] [review]
Patch for comm-central
[Checkin: Comment 8]


Why didn't you copy the Makefile.in part too?
(Assignee)

Comment 10

8 years ago
Because there is no distclean in c-c's Makefile.in (which, btw, is subject of another bug)
(In reply to comment #10)
> Because there is no distclean in c-c's Makefile.in (which, btw, is subject of
> another bug)

http://mxr.mozilla.org/comm-central/source/Makefile.in?mark=107-112#100

Really? [Good catch serge, if anyone wants to push that additional hunk, rs=me; just comment here]
(Assignee)

Comment 12

8 years ago
Created attachment 427729 [details] [diff] [review]
Additional hunk for comm-central
[Checkin: Comment 13]

Here you are
Comment on attachment 427729 [details] [diff] [review]
Additional hunk for comm-central
[Checkin: Comment 13]


http://hg.mozilla.org/comm-central/rev/910c696262b3
Attachment #427729 - Attachment description: Additional hunk for comm-central → Additional hunk for comm-central [Checkin: Comment 13]
(Assignee)

Comment 14

8 years ago
Created attachment 427930 [details] [diff] [review]
Additional patch for js/src [Checkin comment 15]

Sorry, I totally screwed up on this round. This is an additional piece for js/src for the same issue.
Attachment #427930 - Flags: review?(ted.mielczarek)
Attachment #423207 - Attachment is obsolete: true
Attachment #427930 - Flags: review?(ted.mielczarek) → review+
Comment on attachment 427930 [details] [diff] [review]
Additional patch for js/src [Checkin comment 15]

pushed as: http://hg.mozilla.org/mozilla-central/rev/8adfc8e52f6f
Attachment #427930 - Attachment description: Additional patch for js/src → Additional patch for js/src [Checkin comment 15]
Attachment #427324 - Attachment description: Patch for comm-central → Patch for comm-central [Checkin: Comment 8]
Attachment #425223 - Attachment description: Patch v2 → Patch v2 [Checkin: Comment 7]

Updated

3 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.