No problem, and thanks for picking this up. And thanks, Erik, for the review
and advice.
Paul
On 7/14/17, 9:04 AM, "Tim Bell" <tim.b...@oracle.com> wrote:
I can pick this up and sponsor it, but as Erik wrote, I'll be pushing to
jdk10/jdk10.
Tim
On 07/14/17 07:02, Erik Joelsson wrote:
> I realized I spoke too soon. I won't be able to sponsor this anytime
> soon as I'm about to leave on vacation now. Hopefully someone else will
> be able to pick this up for you.
>
> /Erik
>
>
> On 2017-07-14 08:53, Erik Joelsson wrote:
>> This looks good to me. Never mind the regexps, they are fine.
>>
>> I can sponsor the change since it touches configure and will need a
>> corresponding closed change. Do you mind if I push this to jdk10/jdk10
>> instead of hs? That way it will get to hs within a week, but also be
>> in jdk10, while going the other way, it will take much longer before
>> it hits jdk10.
>>
>> /Erik
>>
>> On 2017-07-13 19:24, Hohensee, Paul wrote:
>>> New webrev with two-line change to flags.m4 at line 1129.
>>>
>>> http://cr.openjdk.java.net/~phh/8184022/webrev.03/
>>>
>>> “xno” now means “use build system default”, just as if the switch had
>>> not been used.
>>>
>>> I’m a very inexperienced regex user, so I used what worked first for
>>> me. What would it look like to escape the whole expression or
>>> sub-expressions?
>>>
>>> Paul
>>>
>>> On 7/13/17, 10:04 AM, "Erik Joelsson" <erik.joels...@oracle.com> wrote:
>>>
>>> This looks pretty good. A few points:
>>> * Please use $GREP since configure is doing some work on
>>> finding a good
>>> grep for us. If you want to make the grep patterns more
>>> readable, it's
>>> possible to put the [] escapes around the whole expression, or
>>> at any
>>> level you wish. That way you don't need to repeat them for each
>>> [0-9].
>>> Both styles are used throughout the configure script and I don't
>>> have a
>>> strong preference myself.
>>> * The check for "no" got me thinking. If someone explicitly
>>> sets
>>> --without-macosx-version-max, that probably means they want it
>>> empty.
>>> The reason for doing so would be to override an earlier instance
>>> of the
>>> parameter on the command line (set by some script or automatic
>>> system
>>> that you can't directly influence). This is not a likely usecase
>>> but is
>>> perhaps a more correct action. Sorry for confusing this earlier.
>>> "yes"
>>> is definitely an error though.
>>> I took this patch for a run here and it seems to work as it
>>> should from
>>> the Oracle point of view.
>>> /Erik
>>> On 2017-07-13 17:46, Hohensee, Paul wrote:
>>> > New webrev
>>> >
>>> > http://cr.openjdk.java.net/~phh/8184022/webrev.02/
>>> >
>>> > It includes –with-macosx-version-max format checks (disallows
>>> –without-macosx-version-max) and your jib-profiles.js patch. I put
>>> the checking logic in AC_ARG_WITH based on the code in basics.m4 line
>>> 597 that defines BASIC_SETUP_DEVKIT.
>>> >
>>> > --with-macosx-version-max will fail the format checks and
>>> –without-macosx-version-max will fail the check against ‘xno’.
>>> >
>>> > Paul
>>> >
>>> > On 7/12/17, 1:15 AM, "Erik Joelsson"
>>> <erik.joels...@oracle.com> wrote:
>>> >
>>> > Hello,
>>> >
>>> >
>>> > On 2017-07-12 03:19, Hohensee, Paul wrote:
>>> > > New webrev at
>>> > >
>>> > > http://cr.openjdk.java.net/~phh/8184022/webrev.01/
>>> > For the AC_ARG_WITH, we usually refrain from using the
>>> builtin "if-set,
>>> > if-not-set" parameters and define our own logic to handle
>>> all 4
>>> > possibilities: not set, --with-foobar=value,
>>> --with-foobar and
>>> > --without-foobar. The latter two results in the values
>>> "yes" and "no"
>>> > respectively and in this case those two are invalid and
>>> needs to result
>>> > in errors. Also since we are expecting a very specific
>>> format on the
>>> > input, we need to validate this format so we fail fast
>>> instead of
>>> > getting weird compile errors much later.
>>> >
>>> > My understanding of -mmacosx-version-min is that it sets
>>> > MAC_OS_X_VERSION_MIN_REQUIRED for you so no need to add
>>> that. OTOH, it
>>> > makes it more obvious where this comes from if anyone
>>> stumbles on it in
>>> > the source.
>>> > > I defined a new shell variable MACOSX_VERSION_MAX which
>>> is settable via a new configure switch
>>> –with-macosx-version-max=<version>. Example use:
>>> --with-macosx-version-max=10.12.00. The specified version is passed
>>> via a compiler command line switch, vis
>>> –DMAC_OS_X_VERSION_MAX_ALLOWED=101200 (de-dotted <version>).
>>> > At what point did they introduce the double zeros at the
>>> end? Seems like
>>> > we will need guard the values quite carefully and make
>>> sure we zero pad
>>> > if needed.
>>> > > MACOSX_VERSION_MIN remains hardcoded to 10.7.0, but is
>>> now passed to the compilers via -DMAC_OS_X_VERSION_MIN_REQUIRED=1070
>>> rather than via -DMAC_OS_X_VERSION_MAX_ALLOWED=1070.
>>> > >
>>> > > Tested by attempting builds on OSX 10.12.04.
>>> > >
>>> > > (1) no –with-macosx-version-max: succeeds as expected
>>> because no –DMAC_OS_X_VERSION_MAX_ALLOWED passed to compilers, so
>>> defaults to 10.12.04.
>>> > > (2) –with-macosx-version-max=10.11.00: fails as
>>> expected due to formal parameter type mismatch.
>>> > > (3) –with-macosx-version-max=10.12.00: succeeds as
>>> expected because formal parameter types are the same for all 10.12.xx.
>>> > >
>>> > > It’d be great if you could try it out.
>>> > >
>>> > > Note that successful cases (1) and (3) above provoke
>>> three warnings which I haven’t investigated. Imo, I/we can figure out
>>> how to get rid of these next/later.
>>> > >
>>> > > ld: warning: object file
>>>
(/Users/hohensee/workspaces/jdk10-hs/build/macosx-x86_64-normal-server-release/support/native/java.base//libfdlibm.a)
>>> was built for newer OSX version (10.12) than being linked (10.7)
>>> > > ld: warning: object file
>>>
(/Users/hohensee/workspaces/jdk10-hs/build/macosx-x86_64-normal-server-release/support/native/java.base/libjli_static.a)
>>> was built for newer OSX version (10.12) than being linked (10.7)
>>> > > clang: warning: libstdc++ is deprecated; move to libc++
>>> with a minimum deployment target of OS X 10.9 [-Wdeprecated]
>>> > I believe the warnings for static libs is simply caused
>>> by not adding
>>> > -mmacosx-version-min to the ARFLAGS. Not sure if ar on
>>> mac takes that
>>> > flag though.
>>> >
>>> > The libstdc++ warning seems harder to work around until
>>> we change the
>>> > minimum to 10.9 instead of 10.7.
>>> >
>>> > I would appreciate if you could also include this patch
>>> as part of this
>>> > change to make Oracle builds still behave as before:
>>> >
>>> > diff -r a6c830ee8a67 common/conf/jib-profiles.js
>>> > --- a/common/conf/jib-profiles.js
>>> > +++ b/common/conf/jib-profiles.js
>>> > @@ -436,7 +436,8 @@
>>> > target_os: "macosx",
>>> > target_cpu: "x64",
>>> > dependencies: ["devkit"],
>>> > - configure_args:
>>> concat(common.configure_args_64bit,
>>> > "--with-zlib=system"),
>>> > + configure_args:
>>> concat(common.configure_args_64bit,
>>> > "--with-zlib=system",
>>> > + "--with-macosx-version-max=10.7.0"),
>>> > },
>>> >
>>> > "solaris-x64": {
>>> >
>>> >
>>> > Thanks!
>>> > /Erik
>>> > > Paul
>>> > >
>>> > > On 7/11/17, 2:45 AM, "Erik Joelsson"
>>> <erik.joels...@oracle.com> wrote:
>>> > >
>>> > > The -DMAC_OSX_VERSION_MAX_ALLOWED and
>>> -mmacosx-version-min arguments are
>>> > > used in combination to achieve the same thing. I
>>> chose to use both to
>>> > > really enforce full compatibility with the
>>> specified version. The
>>> > > "official" way of targeting earlier versions of
>>> the OS is just using
>>> > > -mmacosx-version-min. This will however still
>>> accept uses of newer APIs,
>>> > > but at link time, those will be linked with
>>> weak_import. Essentially
>>> > > it's expected that your application should be able
>>> to do without these
>>> > > calls if necessary, at the application level.
>>> While better than not
>>> > > being able to launch at all on the older OS, by
>>> adding
>>> > > -DMAC_OSX_VERSION_MAX_ALLOWED, it becomes a
>>> compile time error if any
>>> > > code tries to use a newer API.
>>> > >
>>> > > As I see it, either we fully enforce this at build
>>> time, or we don't at
>>> > > all. The natural default is to build for the
>>> current host platform. The
>>> > > configure parameter would make it possible to
>>> enforce a minimal
>>> > > compatible OS version that the binaries must be
>>> usable on.
>>> > >
>>> > > (Note that if you propose such a change, I will
>>> need to add the Oracle
>>> > > bit as well, where we use the parameter, which
>>> would need to go in at
>>> > > the same time in common/conf/jib-profiles.js. Also
>>> note that I will be
>>> > > on vacation for 5 weeks starting this weekend so
>>> won't be around to
>>> > > review for most of that time.)
>>> > >
>>> > > /Erik
>>> > >
>>> > >
>>> > > On 2017-07-10 19:48, Hohensee, Paul wrote:
>>> > > > That’s a good idea, though the option would be
>>> --with-macosx-version-max=<n>, right? The minimum is currently
>>> hard-coded and should probably stay that way since there’s likely a
>>> lot of code that depends on it. Let me see what I can come up with.
>>> > > >
>>> > > > Thanks,
>>> > > >
>>> > > > Paul
>>> > > >
>>> > > > On 7/10/17, 10:01 AM, "Erik Joelsson"
>>> <erik.joels...@oracle.com> wrote:
>>> > > >
>>> > > >
>>> > > >
>>> > > > On 2017-07-10 18:09, Hohensee, Paul wrote:
>>> > > > > Hi Erik,
>>> > > > >
>>> > > > > The problem is that the compiler doesn’t
>>> issue a warning in this case, but rather a type-mismatch error on
>>> NSEventMask, so I can’t turn it off. NSUInteger was being used as an
>>> enum, so Apple changed to using a real enum in 10.12 as a matter of
>>> code hygiene. The new code in NSApplicationAWT.m is doing the right
>>> thing by checking MAC_OS_X_VERSION_MAX_ALLOWED.
>>> > > > >
>>> > > > > What particular problem were you trying
>>> to solve? Production, QA and JPRT builds and test runs are done on
>>> the oldest supported OSX version, so any use of newer features should
>>> be detected very early in the test process. Restricting builds to old
>>> OSX versions means that engineers who keep their development boxes up
>>> to date (which they should: security, etc.) can’t use them to do JDK
>>> development.
>>> > > > That's not exactly true. Apple is making it
>>> very hard to stay on older
>>> > > > versions of the OS compared to other OS
>>> vendors. For this reason we are
>>> > > > not always able to stay on a particular
>>> version for Macosx in
>>> > > > particular. We also in general try to avoid
>>> having to fill our build
>>> > > > servers/environments with just the oldest
>>> OSes, because older OSes are
>>> > > > harder to maintain and less convenient to
>>> work with. So instead, we try
>>> > > > to maintain working build environments on
>>> newer OSes that produce
>>> > > > binaries that are compatible with the
>>> oldest we support. So, at least
>>> > > > from Oracle's perspective, we prefer if
>>> builds on different OS versions
>>> > > > produce equivalent binaries when possible.
>>> We certainly don't want to
>>> > > > prevent building on newer OS/compilers.
>>> > > >
>>> > > > If this can't be worked around at the
>>> source level, then perhaps we need
>>> > > > to consider hiding this macro definition
>>> behind a configure option that
>>> > > > we can use internally. I would be open to
>>> that. Something like
>>> > > > --with-macosx-version-min=10.7 which configure
>>> could then translate to
>>> > > > the combination of options currently used.
>>> That way, most openjdk
>>> > > > developers/builders would not need to
>>> suffer this Oracle requirement.
>>> > > >
>>> > > > /Erik
>>> > > > > Thanks,
>>> > > > >
>>> > > > > Paul
>>> > > > >
>>> > > > > On 7/10/17, 1:10 AM, "Erik Joelsson"
>>> <erik.joels...@oracle.com> wrote:
>>> > > > >
>>> > > > > Hello,
>>> > > > >
>>> > > > > I do not agree to removing that
>>> macro. I added those options to help
>>> > > > > guarantee that a build made on a
>>> newer version of macosx would still run
>>> > > > > on the oldest version currently
>>> supported. The macro is not mainly meant
>>> > > > > to be used in our code, but is
>>> picked up by system headers to cause an
>>> > > > > error if any features newer than
>>> 10.7 are used. It may be that we should
>>> > > > > bump it to a newer version of macosx
>>> in JDK 10, but certainly not to 10.12.
>>> > > > >
>>> > > > > It seems to me that we instead need
>>> to ignore the particular warning for
>>> > > > > this case.
>>> > > > >
>>> > > > > /Erik
>>> > > > >
>>> > > > >
>>> > > > > On 2017-07-09 15:26, Hohensee, Paul
>>> wrote:
>>> > > > > > Please review the following change
>>> to get JDK10 to build on OSX 10.12 and above.
>>> > > > > >
>>> > > > > >
>>> https://bugs.openjdk.java.net/browse/JDK-8184022
>>> > > > > >
>>> http://cr.openjdk.java.net/~phh/8184022/webrev.00/
>>> > > > > >
>>> > > > > > I’d very much appreciate a sponsor
>>> for this fix. Imo, successful JDK10 builds on all supported platforms
>>> would be sufficient testing, but please let me know what I can do to
>>> help.
>>> > > > > >
>>> > > > > > Slightly revised from the RFE:
>>> > > > > >
>>> > > > > >
>>> JDK-8182299<https://bugs.openjdk.java.net/browse/JDK-8182299> enabled
>>> previously disabled clang warnings and was intended to also enable
>>> builds on OSX 10 + Xcode 8. Due to a mixup, this code in
>>> jdk/src/java.desktop/macosx/native/libosxapp/NSApplicationAWT.m
>>> > > > > >
>>> > > > > > #if
>>> defined(MAC_OS_X_VERSION_10_12) && \
>>> > > > > > MAC_OS_X_VERSION_MAX_ALLOWED >=
>>> MAC_OS_X_VERSION_10_12 && \
>>> > > > > > __LP64__
>>> > > > > > / 10.12 changed `mask` to
>>> NSEventMask (unsigned long long) for x86_64 builds.
>>> > > > > > - (NSEvent
>>> *)nextEventMatchingMask:(NSEventMask)mask
>>> > > > > > #else
>>> > > > > > - (NSEvent
>>> *)nextEventMatchingMask:(NSUInteger)mask
>>> > > > > > #endif
>>> > > > > > untilDate:(NSDate *)expiration
>>> inMode:(NSString *)mode dequeue:(BOOL)deqFlag {
>>> > > > > >
>>> > > > > > works fine with OSX versions
>>> earlier than 10.12, but fails to compile starting with OSX 10.12 due
>>> to MAC_OSX_VERSION_MAX_ALLOWED being defined on the compile command
>>> line as 10.7.
>>> > > > > >
>>> > > > > > The fix is to remove that
>>> definition, since it places an artificial upper bound on the OSX
>>> version under which JDK10 can be built. A source code search reveals
>>> no uses of MAC_OSX_VERSION_MAX_ALLOWED other than in
>>> NSApplicationAWT.m and hotspot/src/os_cpu/bsd_x86/vm/os_bsd_x86.cpp.
>>> The latter won't be affected by this change, since it checks for a
>>> version > 10.5, which is always true in JDK10.
>>> > > > > >
>>> > > > > > Thanks,
>>> > > > > >
>>> > > > > > Paul
>>> > > > > >
>>> > > > >
>>> > > > >
>>> > > > >
>>> > > >
>>> > > >
>>> > > >
>>> > >
>>> > >
>>> > >
>>> >
>>> >
>>> >
>>>
>>
>
