-
Notifications
You must be signed in to change notification settings - Fork 68
8374522: Mobile: Add iOS as a platform in OperatingSystem.java #44
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
base: master
Are you sure you want to change the base?
Conversation
|
My contributor agreement is under review right now |
|
Hi @KolbyML, welcome to this OpenJDK project and thanks for contributing! We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user KolbyML" as summary for the issue. If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing |
|
❗ This change is not yet ready to be integrated. |
|
/signed |
|
Thank you! Please allow for up to two weeks to process your OCA, although it is usually done within one to two business days. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated! |
|
/signed |
|
Already processed the |
|
Can you create a JBS issue for this? In order for Skara to deal with this PR, it needs to be linked to an issue in JBS (project JDK, OS iOS). |
|
Hi @johanvos it appears that I can't login because I don't have an account, and it looks like getting an account is an invite thing, so I don't think I am able to make an issue, although I want to! If you could help me make an account that would be greatly appreciated! |
|
@KolbyML unfortunately, I can't create/modify accounts on JBS. If you don't have an account, you can still file an issue at https://bugs.java.com/bugdatabase/ |
|
I run this jar on iOS https://github.com/Suwayomi/Suwayomi-Server and here is my iOS project https://github.com/KolbyML/Mangatan/tree/master/bin/mangatan_ios Suwayomi-Server.jar initalizes Kotlin's logger which calls OperatingSystem.iOS, but then I got an error because the modules or static libs I built from openjdk/mobile OperatingSystem didn't have iOS as a member, when I added the member the panic went away Sure I will submit an issue there |
|
I got a bug report made @johanvos https://bugs.java.com/bugdatabase/view_bug?bug_id=JDK-8374522 |
| @ForceInline | ||
| public static boolean isMacOS() { | ||
| return PlatformProps.TARGET_OS_IS_MACOS; | ||
| // Treat iOS as macOS for compatibility with existing libraries unless specific IOS checks exist |
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 needed once the isIOS() call is added? (which is added in this PR)
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.
Also, it looks really odd. Why isn't it something like:
return current() == MACOS || current() == IOS;
or
return PlatformProps.TARGET_OS_IS_MACOS || PlatformProps.TARGET_OS_IS_IOS;
?
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 will test this tomorrow and report back and go from there
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.
Ok I applied the feedback
I don't think this is needed once the isIOS() call is added? (which is added in this PR)
yeah it worked without it, updated the PR
I applied PlatformProps.TARGET_OS_IS_IOS to isIos
|
Can you change the title of this PR into |
It is now in JBS as well: https://bugs.openjdk.org/browse/JDK-8374522 |
Updated 🫡 let me know if I need to do anything else |
magicus
left a comment
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.
LTGM now.

I don't know how but Kotlin calls OperatingSystem.IOS, adding iOS as an OperatingSystem fixed my problems, it had something to do with Kotlin's logging library if I remember correctly during initialization it calls something with OperatingSystem.IOS then it panics because IOS isn't an option in the OperatingSystem Class
Progress
Error
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/mobile.git pull/44/head:pull/44$ git checkout pull/44Update a local copy of the PR:
$ git checkout pull/44$ git pull https://git.openjdk.org/mobile.git pull/44/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 44View PR using the GUI difftool:
$ git pr show -t 44Using diff file
Download this PR as a diff file:
https://git.openjdk.org/mobile/pull/44.diff