On 2015-03-26 15:21, Erik Joelsson wrote:
Hello,
This looks like a nice cleanup.
Thanks! :-)
New webrev that hopefully addresses all issues below is here:
http://cr.openjdk.java.net/~ihse/JDK-8076060-improve-make-bootstrap/webrev.02
Comments inline.
Init.gmk
40: When would topdir not be set?
Answered by a comment now:
# If included from the top-level Makefile then topdir is set, but not when
# recursively calling ourself with a spec.
56, 59, 188, 215: Please break lines if possible
Done.
63: What kind of error messages?
If we have no configuration at all, we warn the user that this is the
case. For this to work, though, we must not *first* get caught by make
claiming that there is no such target. That is, withouth this code, we
would get "No such target" as response to e.g. "make all", instead of
the message "No configuration found, please run configure". However,
I've changed this now (to your off-list suggestion, thanks!) to allow
whatever the user entered as a target, instead of an arbitrary list. I
also improved the comment:
# Without at least a single valid configuration, we cannot extract
any real
# targets. To provide a helpful error message about the missing
configuration
# later on, accept whatever targets the user has provided for now.
ALL_MAIN_TARGETS := $(if $(MAKECMDGOALS), $(MAKECMDGOALS), default)
192: Why does MAKEOVERRIDES need to be reset? Is it automatically
propagated to submake and we prefer to use USER_MAKE_VARS instead?
MAKEOVERRIDES is a special GNU Make variable which contains all the
"FOO=bar" variables set on the command line. Unless cleared, it is
automatically passed down to all further Make calls. In this case, we
don't want to do that since we have a lot of Init.gmk-internal
variables. All relevant user-passed variables are in USER_MAKE_VARS. I
updated the comment to:
# MAKEOVERRIDES is automatically set and propagated by Make to
sub-Make calls.
# We need to clear it of the init-specific variables. The user-specified
# variables are explicitely propagated using $(USER_MAKE_VARS).
194: Should we have "$(INIT_TARGETS): main-init"
No, since we don't want to rotate logs before running reconfigure.
However, I changed the main dependencies thus:
main: $(INIT_TARGETS) main-init
so reconfigure will be run ahead of rotating the logs (this works due to
.NOTPARALLEL:). Seems slightly more logical.
InitSupport.gmk
57: I agree this should be moved somewhere else. We need to split
MakeBase into one file with very basic functions like this. Can be
done later I suppose.
Yes, we definitely need to split MakeBase, and yes, it definitely is a
separate issue. :-)
291: Please break line
spec.gmk.in
62: Please break line
Done and done. :)
/Magnus
