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
