Skip to content

fix macos architecture check #1162

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

Open
evant wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix macos architecture check #1162

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4144d06
fix macos architecture check
evant Nov 21, 2023
1c3d0ef
cache os check and warn if jre and os archs don't match
evant Feb 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 20 additions & 3 deletions paparazzi/src/main/java/app/cash/paparazzi/internal/Renderer.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import java.io.File
import java.nio.file.Paths
import java.util.Locale
import kotlin.io.path.name
import kotlin.jvm.optionals.getOrNull

/** View rendering. */
internal class Renderer(
Expand Down Expand Up @@ -188,12 +189,28 @@ internal class Renderer(
}

private fun getNativeLibDir(): String {
val osName = System.getProperty("os.name").toLowerCase(Locale.US)
Copy link
Collaborator

@jrodbx jrodbx Feb 26, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Author

@evant evant Feb 27, 2024

Choose a reason for hiding this comment

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

my hope with this was to not just give a better error message but to actually make it work

Copy link
Collaborator

@jrodbx jrodbx Feb 28, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@jrodbx jrodbx Mar 1, 2024

Choose a reason for hiding this comment

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

thoughts?

Copy link
Author

@evant evant Mar 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

val osName = System.getProperty("os.name").lowercase(Locale.US)
val osLabel = when {
osName.startsWith("windows") -> "win"
osName.startsWith("mac") -> {
val osArch = System.getProperty("os.arch").lowercase(Locale.US)
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")
Copy link
Collaborator

@jrodbx jrodbx Feb 26, 2024

Choose a reason for hiding this comment

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

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?

Copy link
Author

@evant evant Feb 26, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Author

@evant evant Feb 26, 2024

Choose a reason for hiding this comment

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

Updated to only check once, and it prints a warning if the jre and os architectures don't match

.start()
.inputStream
.bufferedReader()
.lines()
.findFirst()
.getOrNull()
?.lowercase(Locale.US)

if (osArch != null) {
if (osArch.startsWith("i386")) "mac" else "mac-arm"
} else {
// fallback to jre arch and cross fingers it's correct
val jreArch = System.getProperty("os.arch").lowercase(Locale.US)
if (jreArch.startsWith("x86")) "mac" else "mac-arm"
}
}
else -> "linux"
}
Expand Down