RFR: JDK-8218431 Improved platform checking in makefiles

[Magnus Ihse Bursie]

On 2019-02-05 18:31, Magnus Ihse Bursie wrote:
>
>
> 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".

Here is an updated webrev, that uses "ifeq" always (and tests for false 
instead):

http://cr.openjdk.java.net/~ihse/JDK-8218431-improved-platform-checking/webrev.03

I encountered one complicating issue. Statements such as this:

ifneq ($(call isTargetOs, windows)+$(call isTargetCpu, x86_64), true+true)

did not trivially allow themselves to be rewritten without ifneq. So I 
went ahead and fixed the two boolean operators And and Or, that I've 
been missing for a while. So, then I could rewrite it as:

ifeq ($(call And, $(call isTargetOs, windows) $(call isTargetCpu, 
x86_64)), false)

(I tried naming it "and" but it clashed with the GNU Make 4.0 $(and ...) 
function. (Which is not usable for us, since it considers "false" to be 
true.)

I only used And, but created Or as well for completeness.

/Magnus
>
> /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
>>>>
>>>
>