Need official reviewers: hotspot makefile changes from build-infra

[David Holmes]

On 18/10/2012 5:42 AM, Magnus Ihse Bursie wrote:
> First, I think I'm the author of most of these changes but perhaps not
> all, I need to check the webrev more carefully to figure that out. :-)
>
> On 2012-10-17 05:32, David Holmes wrote:
>
>> I got a real sense of deja-vu looking at this. I don't understand what
>> happened to the TEST_IN_BUILD changes - somehow half of that work got
>> left behind ??
>
> Yep. By mistake, what was pushed into mainline was the first attempt at
> the test-in-build fix, not the latest one. :-( I've been mailing
> privately to Kelly on the best way to resolve this, but he's been away
> for a while and haven't had time to sort it out. I'm not sure what's
> happening now, if pushing this patch would result in the correct code or
> not. Even though it will need more work, I think it would be a good
> thing to separate that out from this current patch.

Ok. So I'm assuming that without the rest of this we can't actually set 
TEST_IN_BUILD=false and disable test_gamma ?

>>
>> As for the $(LOG_INFO) changes, I don't get it. This kind of change
>> almost makes sense:
>>
>> - _JUNK_ := $(shell echo >&2 "INFO: ALT_OBJCOPY=$(ALT_OBJCOPY)")
>> + _JUNK_ := $(if $(LOG_INFO),,$(shell echo >&2 "INFO:
>> ALT_OBJCOPY=$(ALT_OBJCOPY)"))
>>
>> I think the intent is that if $(LOG_INFO) is not empty then we will
>> echo the information. But the way it is written we do the echo if
>> LOG_INFO is empty - which I don't understand. But I don't understand
>> all the changes of the form:
>>
>> @echo $(LOG_INFO) ...
>>
>> What is that supposed to do? What value will LOG_INFO have?
>
> I understand your confusion. Here's the trick: If you want to print
> information at the "info" level (that is, you have enabled "info" or any
> more detailed level), then LOG_INFO will be empty. Otherwise, it will
> contain "> /dev/null". That means, if you have a log level less detailed
> than info, all messages at the info level will be hidden.

Ok. The mechanism is surprising but it's not completely unclear what 
this will do: echo when logging at info level is enabled. Though 
"$(LOG_INFO) @echo ..." would look more intuitive. Or change @echo to 
@$(ECHO) and hide this all behind the definition of ECHO (which is what 
happens in other parts of the build - right?)

> But this value
> can also be used as a check. I don't really like to do it that way, but
> the "JUNK/shell echo >&2" stuff gave me no option. (For me that's really
> weird, because "info" is for me like: "this is non-essential, you can
> ignore it if you want" and writing to stderr is like "this is really
> essential, do not ignore this!", and echoing "info" to stderr is...
> well... messed up.)

Right. These FDS related statements are really extended debugging 
information to help everyone figure out why they do, or do not, get the 
debuginfo files, based on different flags and the existance of the 
objcopy tool. I find them somewhat noisy as they get reported multiple 
times. Given you are effectively adding different levels of verbosity to 
the logging I think it would be fine to change these to something more 
"natural" (Of course we run it past Dan :) ). As it is the sense of the 
test and name seem reversed: I don't expect LOG_INFO being empty to 
enable the output.

>>
>> That aside in top.make where you add:
>>
>> + @echo Making vm...
>>
>> shouldn't that be
>>
>> + @echo $(LOG_INFO) Making vm...
>
> The idea here was that this top-level message should be at a "higher"
> log level, so even if you hide all "info" level messages, you should
> still see that we're about to create the vm.

Okay - so how many log levels do we have? Seems to be only two. (Not 
that I really care as I will always build with all logging enabled - and 
expect JPRT to do the same.)

David
-----

> /Magnus