RFR: JDK-8218431 Improved platform checking in makefiles
Magnus Ihse Bursie
magnus.ihse.bursie at oracle.com
Tue Feb 5 17:31:25 UTC 2019
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".
> 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
>>> 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