Closed Bug 905938 Opened 7 years ago Closed 7 years ago

Remove (some) implicit rules from rules.mk

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla26

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(4 files, 6 obsolete files)

As pymake is quite bad at handling them, we can either remove them, or fix pymake. The former is a low hanging fruit.

For the record:

$ cat test.mk 
all: a b

a: foo
b: foo
foo:

%.i: %.c
%.i: %.s

$ python build/pymake/make.py -f test.mk --no-print-directory
('', '.i') test.mk
('', '.i') test.mk
('', '.i') all
('', '.i') all
('', '.i') a
('', '.i') a
('', '.i') foo
('', '.i') foo
('', '.i') b
('', '.i') b

$ hg diff
diff --git a/build/pymake/pymake/data.py b/build/pymake/pymake/data.py
--- a/build/pymake/pymake/data.py
+++ b/build/pymake/pymake/data.py
@@ -620,16 +620,17 @@ class Pattern(object):
     def match(self, word):
         """
         Match this search pattern against a word (string).
 
         @returns None if the word doesn't match, or the matching stem.
                       If this is a %-less pattern, the stem will always be ''
         """
         d = self.data
+        print d, word
         if len(d) == 1:
             if word == d[0]:
                 return word
             return None
 
         d0, d1 = d
         l1 = len(d0)
         l2 = len(d1)
Attachment #791185 - Flags: review?(gps)
Assignee: nobody → mh+mozilla
Attached patch Remove Java rules from rules.mk (obsolete) — Splinter Review
Attachment #791187 - Flags: review?(gps)
Attached patch Remove Java rules from rules.mk (obsolete) — Splinter Review
It turns out these rules have been here forever, and when fennec needed rules to build java source, they added theirs...
Considering those rules in rules.mk are likely not suitable for fennec, if we ever want to actually use central build rules for java, we're probably better off starting from scratch.
Attachment #791188 - Flags: review?(gps)
Attachment #791187 - Attachment is obsolete: true
Attachment #791187 - Flags: review?(gps)
With the four patches applied, we're only left with %/.mkdir.done as implicit rule. The function call count for Pattern.match in pymake.data gets down from > 1M to 67k with my PoC patch for pseudo derecursification.
Attachment #791189 - Flags: review?(gps)
Comment on attachment 791185 [details] [diff] [review]
Use explicit rules for %.i, %.s moc_% and qrc_%

Review of attachment 791185 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/config/rules.mk
@@ +1203,4 @@
>  	$(REPORT_BUILD)
>  	$(CCC) -C -E $(COMPILE_CXXFLAGS) $(COMPILE_CMMFLAGS) $(TARGET_LOCAL_INCLUDES) $(_VPATH_SRCS) > $*.i
>  
> +$(RESFILE): %.res: %.rc

This is the only one that worries me. But, if my BXR output can be trusted, only we only have to worry about https://hg.mozilla.org/mozilla-central/file/default/js/src/Makefile.in#l598. And, since that rule is defined after rules.mk, its recipe takes precedence, so there is no net change.
Attachment #791185 - Flags: review?(gps) → review+
Comment on attachment 791186 [details] [diff] [review]
Cancel GNU make builtin implicit rules by forcing -r instead of adding implicit rules to cancel them out

Review of attachment 791186 [details] [diff] [review]:
-----------------------------------------------------------------

::: config/rules.mk
@@ +1216,5 @@
>  endif
>  endif
>  
> +# Cancel GNU make built-in implicit rules
> +MAKEFLAGS += -r

pymake doesn't support -r. I think you'll need to ifndef guard this.
Attachment #791186 - Flags: review?(gps) → review+
Attachment #791188 - Flags: review?(gps) → review+
Comment on attachment 791189 [details] [diff] [review]
Use explicit rules for {export,libs,tools}_tier_%

Review of attachment 791189 [details] [diff] [review]:
-----------------------------------------------------------------

You'll need to sync these changes to js/src.

Altogether a nice set of patches!

::: config/makefiles/target_export.mk
@@ +16,2 @@
>  	@$(ECHO) "$@"
>  	$(foreach dir,$(tier_$*_dirs),$(call TIER_DIR_SUBMAKE,export,$(dir)))

Alternatively we could just merge this into the CREATE_TIER_RULE define. Actually, I'm not sure what value these target_* .mk files are given us at this time. I may blow things up in the tiers refactor patch I'm working on.
Attachment #791189 - Flags: review?(gps) → review+
Blocks: 906101
Comment on attachment 791185 [details] [diff] [review]
Use explicit rules for %.i, %.s moc_% and qrc_%

Review of attachment 791185 [details] [diff] [review]:
-----------------------------------------------------------------

I get a build failure with this patch during the external tier:

mmx.asm
subtract_sse2.o
yasm -o subtract_sse2.o -f macho64 -rnasm -pnasm -DPIC -I. -I/Users/gps/src/firefox/media/libvpx/ -I/Users/gps/src/firefox/media/libvpx/vpx_ports/   /Users/gps/src/firefox/media/libvpx/vp8/encoder/x86/subtract_sse2.asm
temporal_filter_apply_sse2.o
yasm -o temporal_filter_apply_sse2.o -f macho64 -rnasm -pnasm -DPIC -I. -I/Users/gps/src/firefox/media/libvpx/ -I/Users/gps/src/firefox/media/libvpx/vpx_ports/   /Users/gps/src/firefox/media/libvpx/vp8/encoder/x86/temporal_filter_apply_sse2.asm
make[3]: *** No rule to make target `asm_enc_offsets.s', needed by `asm_enc_offsets.asm'.  Stop.
make[2]: *** [../../media/libvpx_libs] Error 2
make[1]: *** [libs_tier_external] Error 2
make: *** [tier_external] Error 2
Attachment #791185 - Flags: review+
Comment on attachment 791186 [details] [diff] [review]
Cancel GNU make builtin implicit rules by forcing -r instead of adding implicit rules to cancel them out

Review of attachment 791186 [details] [diff] [review]:
-----------------------------------------------------------------

And, uh, for some reason this change causes jar.mn files to always be reprocessed! That's on at least GNU make.
Attachment #791186 - Flags: review+
OS: Linux → All
Hardware: x86_64 → All
Version: 24 Branch → Trunk
(In reply to Gregory Szorc [:gps] from comment #10)
> And, uh, for some reason this change causes jar.mn files to always be
> reprocessed! That's on at least GNU make.

Heh, i see no reason why jar.mn would ever *not* be reprocessed with the current rules. -r or not -r.
(In reply to Gregory Szorc [:gps] from comment #7)
> pymake doesn't support -r. I think you'll need to ifndef guard this.

Actually, pymake doesn't care about things you add to MAKEFLAGS in a makefile. But it's certainly better to guard anyway.
Attachment #791186 - Attachment is obsolete: true
This is kind of ugly, but this entire file is ugly.
Attachment #791635 - Flags: review?(gps)
Attachment #791185 - Attachment is obsolete: true
Another similar one was missing.
Attachment #791645 - Flags: review?(gps)
Attachment #791635 - Attachment is obsolete: true
Attachment #791635 - Flags: review?(gps)
Attached patch Remove Java rules from rules.mk (obsolete) — Splinter Review
Turns out there were used parts, one of which had not much to do with java...
Attachment #791646 - Flags: review?(gps)
Attachment #791188 - Attachment is obsolete: true
Attachment #791634 - Flags: review?(gps) → review+
With more cleanup.
Attachment #792621 - Flags: review?(gps)
Attachment #791646 - Attachment is obsolete: true
Attachment #791646 - Flags: review?(gps)
Comment on attachment 791645 [details] [diff] [review]
Use explicit rules for %.i, %.s, %.res, moc_% and qrc_%

Review of attachment 791645 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/libvpx/Makefile.in
@@ +431,5 @@
>  OFFSET_PATTERN := '^[a-zA-Z0-9_]* EQU'
>  
> +asm_com_offsets.s: asm_com_offsets.c
> +	$(REPORT_BUILD)
> +	$(CC) -S $(COMPILE_CFLAGS) $(TARGET_LOCAL_INCLUDES) $(_VPATH_SRCS)

Please drop a comment saying why the rule in rules.mk isn't sufficient.

Or, perhaps we could introduce a new variable that is appended to CSRCS so we can reuse the rule. e.g.

$(filter %.s,$(CSRCS:%.c=%.s) $(NOCOMPILE_CSRCS:%.c=%.s)): %.s: %.c $(call mkdir_deps,$(MDDEPDIR))
Attachment #791645 - Flags: review?(gps) → review+
Comment on attachment 792621 [details] [diff] [review]
Remove Java rules from rules.mk

Review of attachment 792621 [details] [diff] [review]:
-----------------------------------------------------------------

I love when dead code is eliminated.
Attachment #792621 - Flags: review?(gps) → review+
(In reply to Gregory Szorc [:gps] from comment #18)
> Or, perhaps we could introduce a new variable that is appended to CSRCS so
> we can reuse the rule. e.g.
> 
> $(filter %.s,$(CSRCS:%.c=%.s) $(NOCOMPILE_CSRCS:%.c=%.s)): %.s: %.c $(call
> mkdir_deps,$(MDDEPDIR))

For two files in a single directory, that's a bit of a stretch. And considering we'll have to do something special for that directory anyways, we might as well defer doing anything until then.
(In reply to Mike Hommey [:glandium] from comment #20)
> (In reply to Gregory Szorc [:gps] from comment #18)
> > Or, perhaps we could introduce a new variable that is appended to CSRCS so
> > we can reuse the rule. e.g.
> > 
> > $(filter %.s,$(CSRCS:%.c=%.s) $(NOCOMPILE_CSRCS:%.c=%.s)): %.s: %.c $(call
> > mkdir_deps,$(MDDEPDIR))
> 
> For two files in a single directory, that's a bit of a stretch. And
> considering we'll have to do something special for that directory anyways,
> we might as well defer doing anything until then.

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