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
