RFR: JDK-8218431 Improved platform checking in makefiles

[Magnus Ihse Bursie]

On 2019-02-05 18:01, Erik Joelsson wrote:
> Looks good. I have read through all of them and it does not seem like 
> you got any wrong.
>
> I note that you chose to express negatives as:
>
> ifneq (isTargetFoo, true)
>
> instead of:
>
> ifeq (isTargetFoo, false)
>
> I think I would have preferred the latter, given that the new macros 
> return both true and false. I think true/false is more obvious to the 
> eye than the sneaky 'n' in ifneq, but I don't have a strong opinion so 
> this is fine too.

I did consider this. I was on the verge of actually sending a question 
to the list, asking for input on this choice.

My reasoning was I'm personally a bit blind to the "false" part, from 
all the years of focusing on the ifeq/ifneq, and that "ifeq foo, false" 
feels a bit like the newbie "if (mybool == false)" construct.

But I also agree with your stance. As long as we agree to use one 
standard, I'll be happy to switch to "ifeq ... false". I think the 
important thing is just that we don't have to look out for both "ifneq 
... true" and "ifeq ... false".

/Magnus
>
> /Erik
>
> On 2019-02-05 07:28, Magnus Ihse Bursie wrote:
>> On 2019-02-05 15:49, Magnus Ihse Bursie wrote:
>>> To check for various aspects of the build or target platform, we do 
>>> a lot of checks like:
>>>    ifeq ($(OPENJDK_BUILD_OS_ENV), windows.cygwin)
>>>        ...
>>>
>>> The naming of those platform information variables is a bit 
>>> unfortunate. Yes, we know we're building OpenJDK, so why the 
>>> OPENJDK_ prefix? I've been wanting for a long time to do something 
>>> about this odd prefix, and it became more urgent after the recent 
>>> fix of JDK-8160926, which pushes the matter about unifying the 
>>> naming of build/target variables.
>>>
>>> The solution in this patch is not to rename the variables per se, 
>>> but to introduce an abstraction layer in terms of function calls for 
>>> checking platform aspects.
>>>
>>> This *really* shines when it comes to testing for multiple 
>>> platforms. Previously, we were required to resort to constructs such 
>>> as:
>>>
>>>    ifneq ($(filter $(OPENJDK_TARGET_OS), windows solaris), )
>>>
>>> but this can now be expressed as:
>>>
>>>    ifeq ($(call isTargetOs, windows solaris), true)
>>>
>>> Or this (actually technically broken) horrible example:
>>>
>>>    ifneq (, $(findstring $(OPENJDK_TARGET_OS), linux macosx))
>>>
>>> which I had to read three times before being sure that this was the 
>>> correct way to replace it:
>>>
>>>    ifeq ($(call isTargetOs, linux macosx), true)
>>>
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8218431
>>> WebRev: 
>>> http://cr.openjdk.java.net/~ihse/JDK-8218431-improved-platform-checking/webrev.01
>> ... and here's an updated version that fixes a typo:
>>
>> http://cr.openjdk.java.net/~ihse/JDK-8218431-improved-platform-checking/webrev.02 
>>
>>
>> /Magnus
>>
>>>
>>> /Magnus
>>>
>>