fix macos architecture check #1162
Conversation
|
note: hasn't been fully tested but wanted to make sure the approach made sense first |
use /usr/bin/arch to detect what architecture we are actually running on instead of what arch the jre was built for. This fixes the case of using an intel version of the jvm on arm. Fixes cashapp#1031
evant
force-pushed
the
macos-arch-fix
branch
from
8e9384a
to
4144d06
Compare
| if (osArch.startsWith("x86")) "mac" else "mac-arm" | ||
| // System.getProperty("os.arch") returns the os of the jre, not necessarily of the platform we are running on, | ||
| // try /usr/bin/arch to get the actual architecture | ||
| val osArch = ProcessBuilder("/usr/bin/arch") |
There was a problem hiding this comment.
this method call runs on every Paparazzi test method run, so I suspect this change would introduce a slight perf regression due to frequent shelling out.
is it fair to assume from the tagged issue that the current check would be OK if and only if the proper JDK had been installed?
if so, then I'd propose moving this /usr/bin/arch logic to checkInstalledJvm, changing it to a "valid JDK for OS" check, and guarding that method with a static sentinel (since we shouldn't need to check the installed jvm on every method run either). thoughts?
There was a problem hiding this comment.
is it fair to assume from the tagged issue that the current check would be OK if and only if the proper JDK had been installed?
If you are not running in Rosetta then the original check did work, this pr came about because our work policy locks down what versions of the jdk we can install and they currently don't have the arm version available.
since we shouldn't need to check the installed jvm on every method run either
That is correct, it should only need to be checked once.
There was a problem hiding this comment.
Updated to only check once, and it prints a warning if the jre and os architectures don't match
| else -> "linux" | ||
| } | ||
| return "$osLabel/lib64" | ||
| return "$OsLabel/lib64" |
There was a problem hiding this comment.
| return "$OsLabel/lib64" | |
| return "$osLabel/lib64" |
| @@ -110,3 +111,33 @@ private fun checkInstalledJvm() { | |||
| ) | |||
| } | |||
| } | |||
|
|
|||
| internal val OsLabel: String by lazy { | |||
There was a problem hiding this comment.
| internal val OsLabel: String by lazy { | |
| internal val osLabel: String by lazy { |
| .lines() | ||
| .findFirst() | ||
| .getOrNull() |
There was a problem hiding this comment.
| .lines() | |
| .findFirst() | |
| .getOrNull() | |
| .readLine() |
| .lines() | ||
| .findFirst() | ||
| .getOrNull() | ||
| ?.lowercase(Locale.US) |
There was a problem hiding this comment.
while this input stream could return null, is that likely in practice?
| if (osArch != null && osArch != jreArch) { | ||
| System.err.println("Mismatched architectures, os is:$osArch but the JRE is running with:$jreArch. Installing the correct JDK for your architecture will improve performance.") | ||
| } | ||
| osArch ?: jreArch |
There was a problem hiding this comment.
| if (osArch != null && osArch != jreArch) { | |
| System.err.println("Mismatched architectures, os is:$osArch but the JRE is running with:$jreArch. Installing the correct JDK for your architecture will improve performance.") | |
| } | |
| osArch ?: jreArch | |
| when (osArch) { | |
| null -> jreArch | |
| jreArch -> osArch | |
| else -> throw RuntimeException("Mismatched architectures, os is:$osArch but the JRE is running with:$jreArch. Please installing the correct JDK for your architecture.") | |
| } |
I propose making this fail fast/crash, given the error in #1031 seemed to make this a blocker for others who accidentally installed the wrong JDK.
| @@ -188,16 +188,7 @@ internal class Renderer( | |||
| } | |||
|
|
|||
| private fun getNativeLibDir(): String { | |||
| val osName = System.getProperty("os.name").toLowerCase(Locale.US) | |||
There was a problem hiding this comment.
to my earlier feedback, can we revert this method/Renderer, and instead move the osArch check to Environment under checkInstalledJvm? My rationale is that it seems more appropriate to have this logic live in Environment, and leaving Renderer to assume things are set up correctly.
There was a problem hiding this comment.
my hope with this was to not just give a better error message but to actually make it work
There was a problem hiding this comment.
I get that, but I think trying to make it work can lead to other weird behaviors if the native lib artifact downloaded via dependency resolution isn't the expected one.
There was a problem hiding this comment.
can lead to other weird behaviors
Yeah fair, was trying to spend some time verifying if it would actually work or not, but hadn't reproduced the original issue, at least by running the tests in the repo. Will try to spend more time on if if I can, otherwise, not against your suggestion as a backup, at least makes the issue more clear.
use /usr/bin/arch to detect what architecture we are actually running on instead of what arch the jre was built for. This fixes the case of using an intel version of the jvm on arm.
Fixes #1031