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

RESOLVED FIXED in mozilla1.9.3a2

Status

defect
RESOLVED FIXED
10 years ago
Last year

People

(Reporter: glandium, Assigned: glandium)

Tracking

({dataloss})

Trunk
mozilla1.9.3a2

Firefox Tracking Flags

(status1.9.2 ?)

Details

Attachments

(4 attachments, 1 obsolete attachment)

Assignee

Description

10 years ago
Posted patch Patch (obsolete) — Splinter Review
(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

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

Updated

10 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

10 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

10 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

10 years ago
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

Updated

10 years ago
Attachment #427324 - Flags: review?(kairo) → review+
Pushed c-c patch as: http://hg.mozilla.org/comm-central/rev/9d7af06010f9
Status: ASSIGNED → RESOLVED
Closed: 10 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

10 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]
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

9 years ago
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

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