Right now the .cpp -> .o rules only work if the .cpp file is in the srcdir or the objdir, or in any VPATH directory. Fixing the rules to allow compiling subdir/foo.cpp -> foo.o would help the following cases: 1) Bug 875013 - Removing VPATH - we could list subdir/foo.cpp directly in CPP_SOURCES rather than VPATH = subdir, CPP_SOURCES = foo.cpp 2) Bug 786534 - Removing the install rules for .cpp files (eg: xpcom/glue/standalone/Makefile.in). The moz.build file could just list ../AppData.cpp in CPP_SOURCES rather than making a local copy, so the install rule can go away entirely.

Here's an example patch for rules.mk that allows CPP_SOURCES to work with files outside of srcdir/objdir. I've tested locally both with getting rid of VPATH for CPP_SOURCES, as well as getting rid of the install rule in xpcom/glue/standalone. As an example, in addition to the rules.mk change here, this just requires using definitions like CPP_SOURCES += ['sandbox/SandboxHal.cpp'], and the VPATH = sandbox assignment in hal/Makefile.in can go away. Note: the CPPOBJS and CCOBJS variables in rules.mk are combined, since they can now use the same macro to define the rules. Any feedback would be appreciated, in particular if there's a better solution that doesn't require eval/call :)

Could we just generate explicit rules in the Makefile (based on the moz.build data) rather than doing the complex generic thing with eval/call?

(In reply to Benjamin Smedberg [:bsmedberg] from comment #2) > Could we just generate explicit rules in the Makefile (based on the > moz.build data) rather than doing the complex generic thing with eval/call? That would be a lot of rules. Likely, it would make pymake slower.

Comment on attachment 768582 [details] [diff] [review] Example patch (just rules.mk) Review of attachment 768582 [details] [diff] [review]: ----------------------------------------------------------------- ::: config/rules.mk @@ +361,3 @@ > SOBJS = $(SSRCS:.S=.$(OBJ_SUFFIX)) > +# Filter on what we want because ipc/ipdl/moz.build adds .ipdl/.ipdlh files > +# to CPPSRCS Sounds like that should be fixed instead. (iow, wtf are these files doing in CPPSRCS?)

> That would be a lot of rules. Likely, it would make pymake slower. I really doubt that it would be any slower than $(eval), since that also involves parsing, but in that case the parsing cannot be cached.

Comment on attachment 768582 [details] [diff] [review] Example patch (just rules.mk) Review of attachment 768582 [details] [diff] [review]: ----------------------------------------------------------------- ::: config/rules.mk @@ +1134,5 @@ > + $$(REPORT_BUILD) > + @$$(MAKE_DEPS_AUTO_CXX) > + $$(ELOG) $$(CCC) $$(OUTOPTION)$$@ -c $$(COMPILE_CXXFLAGS) $$(TARGET_LOCAL_INCLUDES) $$(_VPATH_SRCS) > +endef > +$(foreach f,$(CPPSRCS),$(eval $(call compile_cc, $f))) A simpler way to do that is to keep the rules (almost) as they are, but make the target line look like: $(CCOBJS): $(call mkdir_deps,$(MDDEPDIR)) Then, you can add simple dependencies, either by listing them in backend.mk as bsmedberg suggests (foo.o: $(srcdir)/foo.c, etc.), or with eval: define deps $(basename $(notdir $1)).$(OBJ_SUFFIX): $1 endef $(foreach f,$(CPPSRCS) $(CCSRCS),$(eval $(call deps, $(f))) Finally, replace $(_VPATH_SRCS) with $(filter-out $<,$^))

I hacked around this in my gyp->Makefile backend: http://mxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/tools/gyp/pylib/gyp/generator/mozmake.py#81 We could probably just pull that code out into rules.mk. Actually you could probably just write this as: $(OBJS): $(call mkdir_deps,$(addprefix $(CURDIR)/,$(sort $(dir $(OBJS))))) Maybe with some smarts to filter empty dirs out of the list before calling mkdir_deps.

Note that my solution in mozmake.py simply compiles: CPPSRCS := foo/bar/abc.cpp to $(CURDIR)/foo/bar/abc.$(OBJ_SUFFIX).

(In reply to Mike Hommey [:glandium] from comment #4) > Comment on attachment 768582 [details] [diff] [review] > Example patch (just rules.mk) > > Review of attachment 768582 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: config/rules.mk > @@ +361,3 @@ > > SOBJS = $(SSRCS:.S=.$(OBJ_SUFFIX)) > > +# Filter on what we want because ipc/ipdl/moz.build adds .ipdl/.ipdlh files > > +# to CPPSRCS > > Sounds like that should be fixed instead. (iow, wtf are these files doing in > CPPSRCS?) The ipdl fix in bug 868536 should land soon, so the filter is no longer necessary.

Thank you all for the feedback! This new candidate patch uses the approach from https://bugzilla.mozilla.org/show_bug.cgi?id=888016#c6. I did not go with the same gyp->Makefile rules since it won't help with goal 2), which is to remove the custom install rules in cases where we compile the same files from multiple directories (since they would both try to compile to the same .o file). I also did not try to do the rule generation from the moz.build data since the full conversion is not complete yet - some moz.build files still reference Makefile variables in CPP_SOURCES and the like. Obviously we'll need to fix those at some point, but if we support this in rules.mk then we don't have to block until that's done. The main difference between this patch and what glandium suggested is that I did not alter _VPATH_SRCS - there is unfortunately some discrepency between how GNU make and pymake handle $< and $^, which manifests itself when there are dependency lines with no commands. Here is the example Makefile I was using: all: foo.o bar.o baz.o foo.o: foo.cpp something foo.o: @echo compile $< into $@ @echo all deps are $^ bar.o: bar.cpp bar.o: something @echo compile $< into $@ @echo all deps are $^ baz.o: baz.cpp something @echo compile $< into $@ @echo all deps are $^ In GNU make, all 3 cases have $^ report both the .cpp file and the 'something' file as dependencies, though the bar.o case reports 'something' first, so $< does not evaluate to what we want. This is why I removed all the prerequisites from the $(CPPOBJS): and similar rules with command blocks. However in pymake, the foo.o case has empty values for $< and $^. I addressed this in the patch by fixing pymake's $< to match GNU make, so our .cpp file evaluates to the first prerequisite. By putting the foreach/eval for the .cpp dependency before anything else, we don't have to change _VPATH_SRCS. This also helps because _VPATH_SRCS is still used in other rules that don't go through the same eval/call block. Does anyone have any major objections to this approach? If not I'd like to get this out for review sometime next week.

Now that you mention it, there's an open bug about $</$^ in pymake: bug 751076

Now that the $< issue is fixed in pymake, here's the latest patch. Unfortunately when I ran it on try, the config/rules.mk change broke build/unix/elfhack. This is because elfhack/Makefile.in has inject/$(CPU).c in its CSRCS, and expects to create inject/$(CPU).o from it (along with a custom rule to copy inject.c to inject/$(CPU).c). With the rules.mk change, instead of inject/$(CPU).o: inject/$(CPU).c, we had $(CPU).o: inject/$(CPU).c. I tried updating the linker rule to depend on the $(CPU).o file, but apparently it is also loaded at runtime by elfhack.cpp I tried updating elfhack.cpp to also load $(CPU).o instead of inject/$(CPU).o, which ran fine locally, but for some reason segfaulted on try. The current patch includes changes to build/unix/elfhack/Makefile.in so that it builds & runs with the rules.mk change, but it is ugly (and I seemed to have to attach the .o files to export:: in build/unix/elfhack/inject/Makefile.in to get it to build correctly :( ). glandium, joey: Do you have thoughts on how to better handle elfhack with this rules.mk change? If so maybe we can get this through. gps: Since this seems to be taking a lot longer than expected, could we move VPATH to moz.build now and look at removing it later? The lack of VPATH is blocking me for tup at the moment, since the moz.build files don't contain all the information necessary to find CPP_SOURCES and such yet.

Comment on attachment 774889 [details] [diff] [review] Support compilation in subdirectories without VPATH Review of attachment 774889 [details] [diff] [review]: ----------------------------------------------------------------- I'd like to avoid having VPATH in moz.build. But if it's blocking you from working on Tup, then perhaps that's a corner we'll need to cut. If we do add VPATH to moz.build (presumably temporarily), then I'd like to resolve paths in emitter.py if possible. i.e. I'd rather incur the cost of path resolution once, not on every build. That may not be possible in all scenarios. As for this patch, I think it could be avoided if we had a sane way of expressing C++ compilation in moz.build (bug 879837). I'm all for defining that build grammar, using the results to produce non-recursive rules, and then converting existing moz.build/Makefile.in to use that grammar to achieve non-recursive compilation.

Comment on attachment 774889 [details] [diff] [review] Support compilation in subdirectories without VPATH Review of attachment 774889 [details] [diff] [review]: ----------------------------------------------------------------- ::: build/unix/elfhack/Makefile.in @@ -12,5 @@ > > INTERNAL_TOOLS = 1 > > HOST_PROGRAM = elfhack > -NO_PROFILE_GUIDED_OPTIMIZE = 1 Don't remove this. @@ -89,5 @@ > -inject: > - $(NSINSTALL) -D $@ > - > -inject/%.c: inject.c $(call mkdir_deps,inject) > - cp $< $@ If you could make this not require a file copy, it would be even better. That is, it would be great if the build system supported building the same source file as different object files with different build flags. But I don't think it's worth blocking on this.

(In reply to Gregory Szorc [:gps] from comment #13) > If we do add VPATH to moz.build (presumably temporarily), then I'd like to > resolve paths in emitter.py if possible. i.e. I'd rather incur the cost of > path resolution once, not on every build. That may not be possible in all > scenarios. That sounds like a good idea if we need to go that route. > > As for this patch, I think it could be avoided if we had a sane way of > expressing C++ compilation in moz.build (bug 879837). I'm all for defining > that build grammar, using the results to produce non-recursive rules, and > then converting existing moz.build/Makefile.in to use that grammar to > achieve non-recursive compilation. Re-defining the entire build grammar to accommodate one directory that has a bunch of custom rules seems overkill to me. After the grammar is re-defined, you'll still have a bunch of custom rules in elfhack to deal with, so it won't solve the problem.

(In reply to Mike Hommey [:glandium] from comment #14) > Comment on attachment 774889 [details] [diff] [review] > Support compilation in subdirectories without VPATH > > Review of attachment 774889 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: build/unix/elfhack/Makefile.in > @@ -12,5 @@ > > > > INTERNAL_TOOLS = 1 > > > > HOST_PROGRAM = elfhack > > -NO_PROFILE_GUIDED_OPTIMIZE = 1 > > Don't remove this. I took it out because it was redundant with INTERNAL_TOOLS = 1. From config.mk: # No sense in profiling tools ifdef INTERNAL_TOOLS NO_PROFILE_GUIDED_OPTIMIZE = 1 endif I don't mind sticking it back in to be explicit, though. > > @@ -89,5 @@ > > -inject: > > - $(NSINSTALL) -D $@ > > - > > -inject/%.c: inject.c $(call mkdir_deps,inject) > > - cp $< $@ > > If you could make this not require a file copy, it would be even better. > That is, it would be great if the build system supported building the same > source file as different object files with different build flags. But I > don't think it's worth blocking on this. I agree that would be nice to have especially since removing custom copy rules is the 2nd goal of this bug, but elfhack seems to be a special case here. This is something we'll want to keep in mind for re-defining the build grammar, but I think is too much to do for one bug.

Comment on attachment 774889 [details] [diff] [review] Support compilation in subdirectories without VPATH Try seems happy-ish with this: https://tbpl.mozilla.org/?tree=Try&rev=01ac8d304730 I've confirmed via $(warning) that NO_PROFILE_GUIDED_OPTIMIZE is set by config.mk in elfhack.

Comment on attachment 774889 [details] [diff] [review] Support compilation in subdirectories without VPATH Review of attachment 774889 [details] [diff] [review]: ----------------------------------------------------------------- I can't wait to have a better binary grammar for defining all this stuff. r+ conditional on not removing NO_PROFILE_GUIDED_OPTIMIZE. ::: build/unix/elfhack/inject/Makefile.in @@ +37,5 @@ > +GARBAGE += $(CSRCS) > + > +%.$(OBJ_SUFFIX): DEFINES += -DBITS=$(if $(HAVE_64BIT_OS),64,32) > +%.$(OBJ_SUFFIX): CFLAGS := -O2 -fno-stack-protector $(filter -m% -I%,$(CFLAGS)) > +$(CPU)-noinit.$(OBJ_SUFFIX): DEFINES += -DNOINIT Do we still need target specific variable assignments now that we split this into its own Makefile.in? ::: config/rules.mk @@ +1094,5 @@ > +define src_objdep > +$(basename $(notdir $1)).$(OBJ_SUFFIX): $1 $(call mkdir_deps,$(MDDEPDIR)) > +endef > +srcs = $(CSRCS) $(SSRCS) $(CPPSRCS) $(CMSRCS) $(CMMSRCS) $(ASFILES) > +$(foreach f,$(srcs),$(eval $(call src_objdep,$(f)))) Could you just inline $(srcs)? @@ +1148,3 @@ > $(AS) -o $@ $(ASFLAGS) $(LOCAL_INCLUDES) $(TARGET_LOCAL_INCLUDES) -c $< > > +$(CPPOBJS): The cleanup in all these rules is quite nice!

(In reply to Gregory Szorc [:gps] from comment #18) > Comment on attachment 774889 [details] [diff] [review] > Support compilation in subdirectories without VPATH > > Review of attachment 774889 [details] [diff] [review]: > ----------------------------------------------------------------- > > I can't wait to have a better binary grammar for defining all this stuff. > > r+ conditional on not removing NO_PROFILE_GUIDED_OPTIMIZE. Ok, I've added this back in (and in the inject/ sub-directory as well) > > ::: build/unix/elfhack/inject/Makefile.in > @@ +37,5 @@ > > +GARBAGE += $(CSRCS) > > + > > +%.$(OBJ_SUFFIX): DEFINES += -DBITS=$(if $(HAVE_64BIT_OS),64,32) > > +%.$(OBJ_SUFFIX): CFLAGS := -O2 -fno-stack-protector $(filter -m% -I%,$(CFLAGS)) > > +$(CPU)-noinit.$(OBJ_SUFFIX): DEFINES += -DNOINIT > > Do we still need target specific variable assignments now that we split this > into its own Makefile.in? Good point - only the $(CPU)-noinit target-specific variable is needed here. > > ::: config/rules.mk > @@ +1094,5 @@ > > +define src_objdep > > +$(basename $(notdir $1)).$(OBJ_SUFFIX): $1 $(call mkdir_deps,$(MDDEPDIR)) > > +endef > > +srcs = $(CSRCS) $(SSRCS) $(CPPSRCS) $(CMSRCS) $(CMMSRCS) $(ASFILES) > > +$(foreach f,$(srcs),$(eval $(call src_objdep,$(f)))) > > Could you just inline $(srcs)? Yes, done.

Updated patch, r=gps carried forward. Try results: https://tbpl.mozilla.org/?tree=Try&rev=eb1205b1b463

inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/72171f64d589