RFR: JDK-8218431 Improved platform checking in makefiles
Erik Joelsson
erik.joelsson at oracle.com
Wed Feb 6 17:04:48 UTC 2019
Thanks, looks good!
/Erik
On 2019-02-06 07:39, Magnus Ihse Bursie wrote:
>
>
> 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
>>>>>
>>>>
>>
>
More information about the build-dev
mailing list