-
Notifications
You must be signed in to change notification settings - Fork 236
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8344458: [11u] Add initial support for building with XCode 14 #2966
base: master
Are you sure you want to change the base?
Conversation
👋 Welcome back vieiro! A progress list of the required criteria for merging this PR into |
❗ This change is not yet ready to be integrated. |
Webrevs
|
NOTE: The GHA |
17ab59d
to
20c7998
Compare
@vieiro Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information. |
…s to a function without a prototype
f788e53
to
fec6eed
Compare
@vieiro Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems OK to me. I'm not sure we need the doc fixes.
doc/building.html
Outdated
@@ -314,6 +314,7 @@ <h3 id="apple-xcode">Apple Xcode</h3> | |||
<li>Use configure option <code>--with-xcode-path</code>, e.g. <code>configure --with-xcode-path=/Applications/Xcode13.1.app</code> This allows using a specific Xcode version for an OpenJDK build, independently of the active Xcode version by <code>xcode-select</code>.</li> | |||
</ul> | |||
<p>If you have recently (inadvertently) updated your OS and/or Xcode version, and the JDK can no longer be built, please see the section on <a href="#problems-with-the-build-environment">Problems with the Build Environment</a>, and <a href="#getting-help">Getting Help</a> to find out if there are any recent, non-merged patches available for this update.</p> | |||
<p>Experimental support for XCode 14 can be enabled using the <code>--enable-xcode14</code> configuration flag.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this. We don't usually update doc/building.html
for every build change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed in last commit
doc/building.md
Outdated
Experimental support for XCode 14 can be enabled using the `--enable-xcode14` configuration flag. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed in last commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is the right approach. This new flag is called --enable-xcode14
, but its implementation has nothing to do with the compiler version or enabling XCode 14. It simply adds additional compiler flags to disable the warnings. Someone can add --enable-xcode14
with 13 installed and these flags will be added (and presumably the build will fail, as you said these flags are not supported there). Alternatively, someone could have XCode 14 and not know about this flag, and it still fail to build. Also, I fail to see the point of the macosx check, because presumably this is due to a new version of clang that is part of XCode 14, and so the build would fail if someone uses that version of clang on Linux.
If these flags are needed for builds with XCode 14, we should detect that this newer compiler is being used and add them automatically. In fact, ideally, you should not depend on the version at all but test the compiler with the flag and add it if it works. Take a look at FLAGS_SETUP_GCC6_COMPILER_FLAGS
also in flags-cflags.m4
You also don't need to define your own variables and pepper them all over every Makefile. This should be something handled in FLAGS_SETUP_CFLAGS_HELPER
. Look at the area around line 587 where warnings are added to gcc and clang. They are already different there for Linux and MacOS for the JDK. Detecting whether these new flags work and then adding them to WARNING_CFLAGS_JVM
and WARNING_CFLAGS_JDK
looks the right way to go.
I tried with clang 15, 16, 17 & 18 on Linux and did not see similar errors with With warnings as errors on (the default), it fails with:
which I believe is JDK-8287052 Is |
Yep, I came to the same conclusion in the updates_dev mailing list after pushing. The flag was useful to try to compile on XCode 13 and XCode 14 during experiments, but detecting the compiler is probably the way to go. |
I'll give it a try. Yep, it's not ideal (we may not be able to run the tests as in #2967) but it's indeed less disruptive. |
An implementation of JDK-8344458 that adds conditional support for building on
macos
withXCode 14
while keeping compatibility with previousXCode
versions.The PR is separated in three commits for easier review:
--enable-xcode14
configuration flag (which is currently disabled in GHA) and two additional variables:CFLAGS_XCODE14_DEPR_DECLARATIONS
(empty on current XCode versions, set to and-Wno-deprecated-declarations
when--enable-xcode14
is used)CFLAGS_XCODE14_DEPR_NON_PROTOTYPE
(empty on current XCode versions, set to-Wno-deprecated-non-prototype
when--enable-xcode14
is used).CFLAGS_XCODE14_DEPR_DECLARATIONS
to those parts of the codebase that use the deprecatedsprintf
function (i.e., avoidingsprintf
usage errors in XCode 14).CFLAGS_XCODE14_DEPR_NON_PROTOTYPE
to those parts of AWT lib that generate a compilation error (i.e., avoiding thepassing arguments to a function without prototype
).Since the new flag
--enable-xcode14
is not set the build should run exactly the same on the current XCode versions and, consequently, the GitHub checks should pass on all platforms, including the currentmacos-12
&XCode 13.4.1
.Progress
Issue
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk11u-dev.git pull/2966/head:pull/2966
$ git checkout pull/2966
Update a local copy of the PR:
$ git checkout pull/2966
$ git pull https://git.openjdk.org/jdk11u-dev.git pull/2966/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 2966
View PR using the GUI difftool:
$ git pr show -t 2966
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk11u-dev/pull/2966.diff
Using Webrev
Link to Webrev Comment