Hi Erik, Thanks for the review!
On Wed, 2018-11-14 at 09:21 -0800, Erik Joelsson wrote: > Hello, > > On 2018-11-14 03:27, Severin Gehwolf wrote: > > Hi, > > > > Thanks for Erik Joelsson for contributing this temporary?! fix for a > > build issue with LOG=debug. > > > > The actual issue is that on some systems with make 4.x when building > > with LOG=debug MAKE_TEST_TARGETS receives debug output from the > > shell'ed make invocation. This in turn changes ALL_NAMED_TESTS which is > > iterated over in Main.gmk so as to produce test-<name> targets. This > > fails for some of the supposed test names of: "gmake[1]:", "Leaving", > > "directory" or "'<top-dir-path>'". All of them are bogus. > > > > In other words, MAKE_TEST_TARGETS gets value: > > > > make-base java-compilation copy-files idea compile-commands gmake[1]: > > Leaving directory '<top-dir-path>' > > > > instead of: > > > > make-base java-compilation copy-files idea compile-commands > > > > This only happens the second time TestMake.gmk's print-targets is > > executed. Why is it executed (at least) twice? The chain is something > > like this: > > > > Init.gmk => InitSupport.gmk (include) => Main.gmk > > (create-main-targets-include) > > => FindTests.gmk (include) => TestMake.gmk (print-targets) > > => Modules.gmk (include) => ??? => FindTests.gmk (include) => > > TestMake.gmk (print-targets) > > What happens here is that Modules.gmk does a -include on a generated > file (module-deps.gmk), that has a rule for generating it in the same > makefile. Make will check if this file is up to date, if not it will get > generated and then make will restart the current top level makefile from > the top. It seems like make will continue to evaluate the rest of the > current makefile before running the rule however, which is why it > continues down in FindTests.gmk one time, then runs the rule for > module-deps.gmk, then restarts Main.gmk. It's only on the second go > through Main.gmk that the $(shell $(MAKE) -f TestMake.gmk ...) > invocation fails. This is the part that really baffles me. Ugh. Very subtle. > > As to where Modules.gmk depends on FindTests.gmk (or includes it) is a > > mystery to me. > > > > The proposed fix is to add --no-print-directory flag to the main make > > invocation (Main.gmk). > > In my testing it doesn't matter at all what arguments we give make in > the $(shell $(MAKE) -f TestMake.gmk ...) call. What matters is the > makeflags active in the parent process, which is why adding > --no-print-directory in Init.gmk helps. Normally, when running with > LOG=info/warn, we have -s in all make calls, which also implies > --no-print-directory. That is why this only happens for LOG=debug/trace. > > I think this fix is good enough for now, but perhaps we should consider > always running with --no-print-directory even in debug/trace? The > directory we run in isn't really relevant considering how we have > organized our makefiles. That info is basically assuming a normal > recursive makefile structure with a makefile in each src dir. We > basically run everything from the top level make dir. > > We could also reconsider the communications model of the targets in > FindTests.gmk and avoid the $(shell $(MAKE)) call. > > Regarding the patch, I think we need to add a comment explaining it. > Something like: > > # The --no-print-directory is needed to make the call from FindTest.gmk > to Test.gmk work > # with LOG=debug/trace. Done: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8213736/webrev.02/ Thanks, Severin > /Erik > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8213736 > > webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8213736/webrev.01/ > > > > This allows me to build with LOG=debug again. > > > > Thoughts? > > > > Thanks, > > Severin > >
