RFR: JDK-8218431 Improved platform checking in makefiles
erik.joelsson at oracle.com
Tue Feb 5 17:01:26 UTC 2019
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)
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.
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
> ... and here's an updated version that fixes a typo:
More information about the build-dev