jfx
jfx copied to clipboard
openjdk
8278021: Fix warnings in macOS glass native code and treat warnings as errors
Turning on warnings-as-errors for the macOS glass native code. Deprecated declarations are excluded and still appear as warnings.
In the code that tries to locate the application's dock icon there were three instances where NO was being passed into a method that required a pointer to a BOOL, not a BOOL. I suspect the intent was to check that the path pointed to an existing file but not a directory. Since JavaFX has gone this long without screening out directories correctly I decided not to fix that behavior except at the very end.
The only other changes of note are sending some NSNotification objects to delegate API's that require them even though we know they're ignored on the other side. It was the easiest way to get rid of the warning.
Progress
- [x] Change must not contain extraneous whitespace
- [x] Commit message must refer to an issue
- [ ] Change must be properly reviewed
Issue
- JDK-8278021: Fix warnings in macOS glass native code and treat warnings as errors
Reviewing
Using git
Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jfx pull/687/head:pull/687
$ git checkout pull/687
Update a local copy of the PR:
$ git checkout pull/687
$ git pull https://git.openjdk.java.net/jfx pull/687/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 687
View PR using the GUI difftool:
$ git pr show -t 687
Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jfx/pull/687.diff
:wave: Welcome back beldenfox! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.
![bridgekeeper[bot] avatar](https://avatars.githubusercontent.com/u/41768318?v=4 "Published by a pub.dev verified publisher"){kind=small}
Webrevs
- 04: Full - Incremental (ee8877d6)
- 03: Full - Incremental (862067d8)
- 02: Full (1c4c4e96)
- 01: Full (1f70431a)
- 00: Full (25352422)
@kevinrushforth The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).
Yes, the deprecated constants should be fixed as the code is updated. They weren't included in this PR because there's a lot of them and they're all cosmetic.
@beldenfox this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:
git checkout macerrors
git fetch https://git.openjdk.org/jfx master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push
@beldenfox This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!
I'll review this. The large number of warnings can hide a real issue (which I noticed the other day while working on a bug).
@beldenfox This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!
@beldenfox This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.
@beldenfox I let this PR drop off my radar a while ago. Sorry about that!
I've been taking a look at our various warnings in a number of places, and was recently reminded of this by a comment you made in PR #1351 :
I also updated a few deprecated constants so this file no longer generates compiler warnings.
Would you be willing to revive this PR, provided of course, that I actually review it this time? It will likely need to be updated in light of other changes in the past year or so.
@kevinrushforth I'd love to revive this PR! The keyboard related bugs are settling down so I was planning to do some cleanup of the Mac code this cycle. I'll take another look at this.
Re-opening this PR. Instead of disabling deprecation warnings globally I added pragmas to the relevant files. This will make it easier to continue the clean-up; someone can file a bug against a specific file, remove the pragma, and fix it up without having to wade through the warnings generated by the other files.
The pre-submit error is due to a call to [NSApp activate] which is only available in macOS 14.0 and up. My local builds use a 14.x SDK but the pre-submit tests are built with 13.3. It's an easy fix but I first need to find out why the SDK matters; the minimum target is 11.0 so I should have seen a warning.
Ouch.
We're targeting a minimum macOS version of 11.0 but [NSApp activate] is only available on 14.0 and above. Xcode 15 should issue a warning about this but doesn't if this is the only version warning in the file. I've tried this with a standalone app and also tweaked the Glass code and the result is the same; I get no warning unless I add some other 14.0-specific call in the same file. At that point I'll get a warning about the other violation in addition to the [NSApp activate] call.
Adding an @available check around this is the right thing to do but not enough to appease the pre-submit bot which is building with the 13.3 SDK. To clean this up either the bot needs to be upgraded or I need to add a localized pragma to disable this check.
We're targeting a minimum macOS version of 11.0 but
[NSApp activate]is only available on 14.0 and above. Xcode 15 should issue a warning about this but doesn't if this is the only version warning in the file.
Sigh. I'm not much a fan of how Apple handles runtime version checks, and this seems like one more indication of that.
Adding an
@availablecheck around this is the right thing to do but not enough to appease the pre-submit bot which is building with the 13.3 SDK.
Technically the @available check is redundant (the only place that requiresActivation is set to YES is already in an @available check), but I can see how the compiler wouldn't know that. Adding it around the use site seems fine; too bad it is insufficient.
To clean this up either the bot needs to be upgraded
Definitely not. This matches our production build environment. We use Xcode 14.3.1, which includes MacOSX13.sdk.
or I need to add a localized pragma to disable this check.
So basically, a pragma to workaround a limitation of Xcode. This might be the best option if it isn't too ugly.
Btw, I ran this PR through our CI and got the expected error:
Starting process 'command 'jfx/buildSrc/build/build-tools/devkit-macosx-Xcode14.3.1+1.0.tar/Xcode/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang''. Working directory: jfx/modules/javafx.graphics Command: jfx/buildSrc/build/build-tools/devkit-macosx-Xcode14.3.1+1.0.tar/Xcode/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang -target x86_64-apple-macos-11 -mmacosx-version-min=11.0 -isysroot jfx/buildSrc/build/build-tools/devkit-macosx-Xcode14.3.1+1.0.tar/Xcode/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.sdk -iframeworkjfx/buildSrc/build/build-tools/devkit-macosx-Xcode14.3.1+1.0.tar/Xcode/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.sdk/System/Library/Frameworks -arch x86_64 -I/Users/java_re/jenkins/workspace/jfx-submit/label/mac-13-x64/jfx/bootjdk/jdk-21.0.2.jdk/Contents/Home/include -I/Users/java_re/jenkins/workspace/jfx-submit/label/mac-13-x64/jfx/bootjdk/jdk-21.0.2.jdk/Contents/Home/include/darwin -std=c99 -c -O3 -DNDEBUG -DGL_SILENCE_DEPRECATION -DMACOS_MIN_VERSION_MAJOR=11 -DMACOS_MIN_VERSION_MINOR=0 -Werror -Ijfx/modules/javafx.graphics/build/gensrc/headers/javafx.graphics -Ijfx/modules/javafx.graphics/src/main/native-glass/mac -Ijfx/modules/javafx.graphics/src/main/native-glass/mac/a11y -o jfx/modules/javafx.graphics/build/native/glass/mac/GlassApplication.obj jfx/modules/javafx.graphics/src/main/native-glass/mac/GlassApplication.m
Successfully started process 'command 'jfx/buildSrc/build/build-tools/devkit-macosx-Xcode14.3.1+1.0.tar/Xcode/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang''
jfx/modules/javafx.graphics/src/main/native-glass/mac/GlassApplication.m:304:24: error: instance method '-activate' not found (return type defaults to 'id') [-Werror,-Wobjc-method-access]
[NSApp activate];
^~~~~~~~
jfx/buildSrc/build/build-tools/devkit-macosx-Xcode14.3.1+1.0.tar/Xcode/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX13.sdk/System/Library/Frameworks/AppKit.framework/Headers/NSApplication.h:158:12: note: receiver is instance of class declared here
@interface NSApplication : NSResponder <NSUserInterfaceValidations, NSMenuItemValidation, NSAccessibilityElement, NSAccessibility>
^
1 error generated.
So basically, a pragma to workaround a limitation of Xcode.
I wouldn't describe this as a limitation of Xcode. After all, the glass code is trying to access a 14.0 API using a 13.3 SDK. If the language was C++ the call wouldn't be in the headers and the code wouldn't compile.
If we launch a jfx22 app on macOS 13 in theory we're sending NSApp an activate selector that it doesn't recognize. Or maybe it does? It's possible that this is an old internal call that Apple finally documented in 14.0. That would explain why they would be lenient about issuing a warning. I'll run some tests.
So basically, a pragma to workaround a limitation of Xcode.
I wouldn't describe this as a limitation of Xcode. After all, the glass code is trying to access a 14.0 API using a 13.3 SDK. If the language was C++ the call wouldn't be in the headers and the code wouldn't compile.
In C++ we would need to use reflection (dlsym), so I can see your point.
If we launch a jfx22 app on macOS 13 in theory we're sending NSApp an
activateselector that it doesn't recognize. Or maybe it does? It's possible that this is an old internal call that Apple finally documented in 14.0. That would explain why they would be lenient about issuing a warning. I'll run some tests.
No, we're not. The @available generates a runtime check so that code is executed only if the runtime platform is macOS 14.x or later.
No, we're not. The
@availablegenerates a runtime check so that code is executed only if the runtime platform is macOS 14.x or later.
Right, forgot about the other @available check.
This is now compiling on 13.3 and I was able to do a Robot test run on macOS 14 with only one failure that's also present in the master branch. I still don't know why the compiler doesn't flag the activate call, in macOS 13 NSApp does not respond to that selector and will throw an exception if it's sent.
This now passes on our CI build systems. I'll review it next week some time. Most of the changes are trivial, but there are a couple I want to double-check that there is no change in behavior.
@beldenfox This change now passes all automated pre-integration checks.
ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.
After integration, the commit message for the final commit will be:
8278021: Fix warnings in macOS glass native code and treat warnings as errors
Reviewed-by: kcr, aghaisas
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.
At the time when this comment was updated there had been 13 new commits pushed to the master branch:
- 9a06bf9c5bce40cf842ddf25fe3360c366c3156c: 8322748: Caret blinking in JavaFX should only stop when caret moves
- ee8633cb6d19b6da7bf32ad3cdee31261a7cf458: 8089373: Translation from character to key code is not sufficient
- 1fb56e333bc65860cc1abeebd1cbb01cd8b8e5f3: 8323880: Caret rendered at wrong position in case of a click event on RTL text
- 2754939556aa767a298fcf24f5caaa277351a4d7: 8309374: Accessibility Focus Rectangle on ListItem is not drawn when ListView is shown for first time
- 48be7d36d5033d215d585619f34c2dde518e4b53: 8324182: Deprecate for removal SimpleSelector and CompoundSelector classes
- 49d7d52fdf1c7341538e5bf4bd1a841dcc85c906: 8325798: Spinner throws uncatchable exception on tab out from garbled text
- f379eae65aab963779d7b7335cbade810f8efe82: 8325873: Update JDK_DOCS property to point to JDK 21 docs
- e2f42c5b71ef061c614540508fbac3fb610b1ae3: 8325154: resizeColumnToFitContent is slower than it needs to be
- de0255d6b6da52a7af367319f883afb08de5182a: 8307117: TextArea: wrapText property ignored when changing font
- a7f6de8d13fd7a7e37ca80b28badcb24566153d8: 8325258: Additional WebKit 617.1 fixes from WebKitGTK 2.42.5
- ... and 3 more: https://git.openjdk.org/jfx/compare/12816b57ffd1ffad57d008f7195666ad4e9400bb...master
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.
➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.
Going to push as commit e0b88bc7cfede46afe28cbb4a2e333df933b5100.
Since your change was applied there have been 13 commits pushed to the master branch:
- 9a06bf9c5bce40cf842ddf25fe3360c366c3156c: 8322748: Caret blinking in JavaFX should only stop when caret moves
- ee8633cb6d19b6da7bf32ad3cdee31261a7cf458: 8089373: Translation from character to key code is not sufficient
- 1fb56e333bc65860cc1abeebd1cbb01cd8b8e5f3: 8323880: Caret rendered at wrong position in case of a click event on RTL text
- 2754939556aa767a298fcf24f5caaa277351a4d7: 8309374: Accessibility Focus Rectangle on ListItem is not drawn when ListView is shown for first time
- 48be7d36d5033d215d585619f34c2dde518e4b53: 8324182: Deprecate for removal SimpleSelector and CompoundSelector classes
- 49d7d52fdf1c7341538e5bf4bd1a841dcc85c906: 8325798: Spinner throws uncatchable exception on tab out from garbled text
- f379eae65aab963779d7b7335cbade810f8efe82: 8325873: Update JDK_DOCS property to point to JDK 21 docs
- e2f42c5b71ef061c614540508fbac3fb610b1ae3: 8325154: resizeColumnToFitContent is slower than it needs to be
- de0255d6b6da52a7af367319f883afb08de5182a: 8307117: TextArea: wrapText property ignored when changing font
- a7f6de8d13fd7a7e37ca80b28badcb24566153d8: 8325258: Additional WebKit 617.1 fixes from WebKitGTK 2.42.5
- ... and 3 more: https://git.openjdk.org/jfx/compare/12816b57ffd1ffad57d008f7195666ad4e9400bb...master
Your commit was automatically rebased without conflicts.
@beldenfox Pushed as commit e0b88bc7cfede46afe28cbb4a2e333df933b5100.
:bulb: You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.