Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Prefer system-wide JRE installation if compatible with shipped IDE (workaround for problems on MacOSX and external monitor) #10234

Closed
wants to merge 2 commits into from
Closed

Prefer system-wide JRE installation if compatible with shipped IDE (workaround for problems on MacOSX and external monitor) #10234

wants to merge 2 commits into from

Conversation

facchinm
Copy link
Member

To make Java IDE more future-proof (and avoid these kind of issues) one of the possible actions is to reverse the priority of the bundled JRE.
Doing this allows us to keep bundling OpenJDK (or AdoptJDK) with all its bugs (or older versions of Oracle's JRE from before the license change), while using the system-wide JRE if it matches the requirements.

Linux version of the patch still uses bundled JRE if the system one self reports as openjdk to avoid strange issues with distro provided JREs. I'm not 100% sure it's the right decision but we can test it and see if it makes sense

@alranel @cmaglie @PaulStoffregen

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

  1. Does your submission pass tests?
  2. Have you lint your code locally prior to submission?

Changes to Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully ran tests with your changes

facchinm and others added 2 commits May 17, 2020 13:50
@facchinm
Prefer system-wide JRE installation if it qualifies
93ba322 
Doing this allows us to keep bundling OpenJDK (or AdoptJDK) with all its bugs,
while using the systemwide JRE (normally Oracle's) if it matches the requisites.

On MacOS, these lines from appbundler should be reversed (and probably the version checked more strictly)
https://github.com/arduino/appbundler/blob/ae/appbundler/native/main.m#L159-L175
@cmaglie
Updated mac appbundler to 1.0.1-arduino
8904aba 
This version will use system-wide installed JRE and fallback to bundled
JRE if no system JVM is installed.
@PaulStoffregen
Copy link
Contributor

PaulStoffregen commented May 18, 2020 •
edited
Loading

Looks like version 8u252 may have fixed the terrible OpenJDK bugs we've been seeing. I created a test copy of Arduino 1.8.12 with the 8u242 replaced with version 8u252. Just got feedback on the forum from a user who was experiencing the crashes that 8u252 seems to fix the problem.

https://forum.arduino.cc/index.php?topic=661342.msg4607373#msg4607373

matthijskooijman
matthijskooijman reviewed May 18, 2020
View reviewed changes
Copy link
Collaborator

@matthijskooijman matthijskooijman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The commit message says "while using the systemwide JRE (normally Oracle's) if it matches the requisites.", but what requisites are these? On Linux apparently "not OpenJDK", but what are these on Windows? Is that just <runtimeBits>32</runtimeBits> maybe?

build/linux/dist/arduino
export JAVA_TOOL_OPTIONS=`echo $JAVA_TOOL_OPTIONS | sed 's|-javaagent:/usr/share/java/jayatanaag.jar||g'`

JAVA=java
if [ -x "$APPDIR/java/bin/java" ]; then

#check systemwide java version, prefer Oracle jre
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment seems confusing. This does not prefer oracle jre, AFAICS it prefers the system-wide JRE, except when its OpenJDK, then it prefers the bundled version (which is also OpenJDK IIUC, but then at least a version with known problems :-p).

build/linux/dist/arduino
fi

if [[ -x "$APPDIR/java/bin/java" && x$SYSTEM_OPENJDK != "x" ]]; then
echo "Using bundled Java JRE"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this have an else that says "Using system Java JRE"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense

@PaulStoffregen
Copy link
Contributor

More good news. A 2nd user experiencing the Mac crashes has confirmed 8u252 fixes the problem.

https://forum.pjrc.com/threads/60904-Teensyduino-crashes-constantly-on-Catalina?p=240069&viewfull=1#post240069

@facchinm
Copy link
Member Author

The commit message says "while using the systemwide JRE (normally Oracle's) if it matches the requisites.", but what requisites are these? On Linux apparently "not OpenJDK", but what are these on Windows? Is that just <runtimeBits>32</runtimeBits> maybe?

Both appbundler and launch4j already enforce that the "fallback" (or after this patch, the preferred one) are java8 with the correct "bitness" (only applies on Win since macOS has been 64bit only for such a long time)

@facchinm
Copy link
Member Author

More good news. A 2nd user experiencing the Mac crashes has confirmed 8u252 fixes the problem.

https://forum.pjrc.com/threads/60904-Teensyduino-crashes-constantly-on-Catalina?p=240069&viewfull=1#post240069

This is great news! @cmaglie time to spin up the JRE compile task?

@PaulStoffregen
Copy link
Contributor

PaulStoffregen commented May 18, 2020 •
edited
Loading

FWIW, I did it the lazy way, just downloaded "OpenJDK8U-jre_x64_mac_hotspot_8u252b09.tar.gz" from the AdoptOpenJDK site. No compile needed, as they're now building it for SDK 10.10 which passes Apple's notarization requirements.

I just extracted it, renamed the folder to "jre1.8.0_242.jre", and put it in 1.8.12's Contents/PlugIns folder. Then I signed, notarized and stapled the whole IDE. No rebuilding the JRE needed. :)

@PaulStoffregen
Copy link
Contributor

Well, and some not-so-awesome news. One of those users now reports the fix is only partial. Apparently still crashes when used on a 2nd monitor, but at least now seems to be stable on the primary display. Details on the forum.

@cmaglie
Copy link
Member

cmaglie commented May 18, 2020

FWIW, I did it the lazy way, just downloaded "OpenJDK8U-jre_x64_mac_hotspot_8u252b09.tar.gz" from the AdoptOpenJDK site. No compile needed, as they're now building it for SDK 10.10 which passes Apple's notarization requirements.

Ohh finally! I tried the 8u252 as soon as it was published, but it wasn't build yet with latest Apple SDK, I guess they updated the builder later.

Well, and some not-so-awesome news. One of those users now reports the fix is only partial. Apparently still crashes when used on a 2nd monitor, but at least now seems to be stable on the primary display. Details on the forum.

I tested 8u252 (with the help of my collegue that was experiencing the issue on his mac) and it was still crashing, so I have to add another +1 on the "negative" side...

@facchinm facchinm mentioned this pull request May 22, 2020
Open
@facchinm facchinm added this to the Release 1.8.13 milestone Jun 3, 2020
@cmaglie
Copy link
Member

cmaglie commented Jun 4, 2020 •
edited
Loading

Here a preview build for MacOSX, it's signed+notarized:
https://downloads.arduino.cc/arduino-1.8.13-preview-macosx.zip

This build bundles the latest OpenJDK 8u252 from AdoptOpenJDK, that solves the external monitor issue in some cases (not all). In case you still experience the issue, with this release the workaround is to install the Oracle JRE 8 in your system after downloading it from this website https://www.java.com/download, once installed run the IDE again and it should now use the Oracle JRE instead of the bundled one hopefully solving the problem.

@arduino arduino deleted a comment from ArduinoBot Jun 4, 2020
@cmaglie cmaglie changed the title Prefer system-wide JRE installation if compatible with shipped IDE Prefer system-wide JRE installation if compatible with shipped IDE (workaround for problems on MacOSX and external monitor) Jun 5, 2020
@cmaglie
Copy link
Member

cmaglie commented Jun 5, 2020

@makermelissa @emha69 @VASAPOL @ccohen20 @kako21 @ghost @CharlieBouyaka
I know from previous issues that you have got crash of the Arduino IDE with MacOSX and external monitor setup. May I ask you to test the version here https://downloads.arduino.cc/arduino-1.8.13-preview-macosx.zip and to tell me:

  1. If running the Arduino IDE above solves the second-monitor-crash issue.
  2. If the issue is not solved -> install the Oracle Java Virtual Machine downloaded from https://www.java.com/download and re run the IDE afterwards

Thanks!

@emha69
Copy link

emha69 commented Jun 5, 2020 via email •
edited by cmaglie
Loading

@cmaglie
Copy link
Member

cmaglie commented Jun 5, 2020

@emha69 thanks you for testing, that's very bad news!
May you open a terminal and run this command:

"/Applications/Dev Tools/Arduino-Beta.app/Contents/MacOS/Arduino"

It should print some debugging output, if you can copy & paste here, thanks.

@emha69
Copy link

emha69 commented Jun 5, 2020 via email •
edited by per1234
Loading

@cmaglie
Copy link
Member

cmaglie commented Jun 5, 2020

github eat the attachment, please send to c.maglie AT arduino.cc

@kako21
Copy link

kako21 commented Jun 6, 2020 via email

@fgreen
Copy link

fgreen commented Jun 7, 2020

In case it is helpful, I tried running the version from https://downloads.arduino.cc/arduino-1.8.13-preview-macosx.zip. It crashed, so I downloaded the linked Java updater (jre-8u251-macosx-x64) and removed java installation via that tool instead of installing it. That ran for a few minutes before crashing. I then tried running the updater again and installing it, but it crashed again.
I'm running OSX 10.15.4 on a Macbook Pro with an external display.

@cmaglie
Copy link
Member

cmaglie commented Jun 9, 2020

ok, thanks to these test and the logs sent by @emha69 I've made some more test and I discovered that:

IDE Version JRE Result
1.8.12 Oracle JRE 8u251 Crash
1.8.12 Oracle JRE 8u191 Crash
1.8.10 Oracle JRE 8u191 Works (latest known working version)
1.8.12 Open JDK 8u252 Crash
1.8.10 Open JDK 8u252 Works (!!!)

At this point the only possible explanation is that something happened between 1.8.12 and 1.8.10 so I started a git bisection and the breaking commit is 76699fd:

:~/Code/arduino$ git bisect good
76699fd4644be083937ae4812bedefe3e22f3bd9 is the first bad commit
commit 76699fd4644be083937ae4812bedefe3e22f3bd9
Author: Cristian Maglie <c.maglie@arduino.cc>
Date:   Tue Dec 17 14:53:57 2019 +0100
    macosx notarization: signed appbundler
:040000 040000 b519702aeb47edaa3878dcdcef79739ba598619f a1568e013da85c191a44227ceaf4e1e3c1f28278 M	build

Indeed, if I replace appbundler with the previous version it works again! (but we cannot notarize the app anymore...)

So to recap: there is something with the new appbundler that makes the app crash but we cannot use the old appbundler becuase it can't be notarized. Tomorrow I will make some more investigation, just wanted to make a heads up here too just in case someone has some ideas...

The latest appbundler has been build from here: https://github.com/arduino/appbundler/tree/ae

@missingnoodle
Copy link

Hello All,
I have been watching this issue on occasion, and learning, at the same time. I did not know about Git bisect. At any rate, after the post on the appbundler, I went to it's home on GitHub (https://github.com/TheInfiniteKind/appbundler) and noticed that there appear to be more recent edits to the code that may be relevant if merged into your fork. In particular the commit at (TheInfiniteKind/appbundler@7e624ce) may be worth a quick look.

Regards,
Tami Black

@cmaglie cmaglie mentioned this pull request Jun 10, 2020
Open
@per1234 per1234 mentioned this pull request Jun 11, 2020
Closed
cmaglie added a commit to arduino/appbundler that referenced this pull request Jun 11, 2020
@cmaglie
Build against MacOSX SDK 10.9
9db7000 
Crash have been reported if app launcher is build with the latest SDK.

See github.com/arduino/Arduino/pull/10234
@cmaglie
Copy link
Member

cmaglie commented Jun 11, 2020

@emha69 @missingnoodle @kako21 @fgreen

Can you please test this version:
http://downloads.arduino.cc/arduino-1.8.13-preview2-macosx.zip
(please note this is -preview2)

@missingnoodle
Copy link

For me, this build worked! Thank you!

By works I mean:
A whole bunch of test cases, including opening/closing/moving single/multiple windows on a single monitor, multiple monitors, etc. Including running the serial monitor on both the same and different monitors as the currently executing sketch.

The hardware I tested on:
MacBookPro (13in, 2019 w/ Intel Iris Plus Graphics 655 1536 MB)

Display Configuration (4 displays total)
Two External displays + iPad (via Sidecar) + laptop monitor
Note: the two external displays are being driven by the DisplayLink driver (5.2.4)

Unfortunately there is one variable that is in play for my testing, that may play a part in the success, but I cannot verify.

  1. I am running the test against (Catalina 10.15.6 beta 2 19G46c) - not the official released build

Regards,
Tami

@missingnoodle
Copy link

Regarding, testing against the 10.15.6 beta 2 release. At least I can verify, that having built the app bundler against the 10.9 SDK, hasn't seemed to have any side effects/regressions, in my brief testing, against the latest and greatest OS X beta build.

@cmaglie
Copy link
Member

cmaglie commented Jun 11, 2020

Thanks @missingnoodle,
I must say that your last comment lead me to have a look at the upstream appbundler repository.

Even if the commit you linked was not related, I finally ended up here TheInfiniteKind/appbundler#70 and the discussion made me think about this solution. So... yeah, a very small and simple patch but really hard to find.

@missingnoodle
Copy link

missingnoodle commented Jun 11, 2020 •
edited
Loading

No problem. Isn't almost always the case, a solution, being small and simple, yet, difficult to find/resolve... I'm glad my post helped lead you to thinking about the solution you presented. As I am not a committer, nor familiar with the codebase, I did not want to presume anything, but hoped my referencing the commit that I did would lead to a positive result.

@emha69
Copy link

emha69 commented Jun 11, 2020 via email •
edited by cmaglie
Loading

@allthemoose
Copy link

Hi, Replying here instead of #10349 because It is duplicate and closed. Tested the Preview provided and works for me. Confirmed the nightly build still crashes and then tested this all working well. Tried minimise maximise on each screen moving between and scrolling while between all three monitors. Macbook Pro 11,1 Iris graphics 2 additional monitors over USB3 hub.
Good job. OSX Catalina 10.15.5.

@kako21
Copy link

kako21 commented Jun 12, 2020 via email •
edited by cmaglie
Loading

@cmaglie cmaglie mentioned this pull request Jun 12, 2020
Merged
@facchinm
Copy link
Member Author

Superseded by #10357

@facchinm facchinm closed this Jun 12, 2020
@cmaglie cmaglie deleted the jre_as_fallback branch June 12, 2020 10:19
@per1234 per1234 mentioned this pull request Jan 17, 2021
Closed
@per1234 per1234 added Type: Bug OS: OSX Specific to the Mac OS X (macOS) version of the Arduino IDE Type: Duplicate Another item already exists for this topic labels Sep 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthijskooijman matthijskooijman matthijskooijman left review comments

Assignees
No one assigned
Labels
OS: OSX Specific to the Mac OS X (macOS) version of the Arduino IDE Type: Bug Type: Duplicate Another item already exists for this topic
Projects
None yet
Milestone
Release 1.8.13
Development

Successfully merging this pull request may close these issues.
10 participants
@facchinm @PaulStoffregen @cmaglie @emha69 @kako21 @fgreen @missingnoodle @allthemoose @matthijskooijman @per1234