Closed Bug 541767 Opened 15 years ago Closed 14 years ago

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


(Firefox Build System :: General, defect)

Not set


(status1.9.2 ?)

Tracking Status
status1.9.2 --- ?


(Reporter: glandium, Assigned: glandium)


(Keywords: dataloss)


(4 files, 1 obsolete file)

Attached patch Patch (obsolete) — Splinter Review
(I think this is the same on trunk)

I found out about this issue because config/ and config/ 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/ and config/ 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/ and config/ when they exist.
Attachment #423207 - Attachment is patch: true
Attachment #423207 - Attachment mime type: application/octet-stream → text/plain
Attachment #423207 - Flags: review?(benjamin)
Attachment #423207 - Flags: review?(benjamin) → review?(ted.mielczarek)
Comment on attachment 423207 [details] [diff] [review]

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-
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.
My only rationale for trying to make it (slightly) faster is the comment justifying the creation of the and 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.
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
Keywords: checkin-needed
Attachment #427324 - Flags: review?(kairo) → review+
Pushed c-c patch as:
Closed: 14 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 part too?
Because there is no distclean in c-c's (which, btw, is subject of another bug)
(In reply to comment #10)
> Because there is no distclean in c-c's (which, btw, is subject of
> another bug)

Really? [Good catch serge, if anyone wants to push that additional hunk, rs=me; just comment here]
Comment on attachment 427729 [details] [diff] [review]
Additional hunk for comm-central
[Checkin: Comment 13]
Attachment #427729 - Attachment description: Additional hunk for comm-central → Additional hunk for comm-central [Checkin: Comment 13]
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:
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]
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.