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.

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

RQA-2830 App cannot analyze due to mismatched build architecture of dmg and bundled python #15618

Closed
wants to merge 4 commits into from

Conversation

y3rsh
Copy link
Member

@y3rsh y3rsh commented Jul 10, 2024

Overview

  • https://opentrons.atlassian.net/browse/RQA-2830
  • macos-11 GitHub runner was intel and now we use macos-latest that is arm doc
  • specify --x64 argument to electron-builder
  • per doc x64 python should be installed so the pip install will be the right platform. I'm not confident in this.
  • see comment, we need to use the python we download on mac to do the pip install so the installed packages match the platform correctly

Test Plan

  • build locally and see that it works
  • watch CI and install the built package
  • validate that once the package dmg is x64 that analysis works (python has packages with the right arch)

Review requests

  • This the right approach?

Risk assessment

High because our .dmg currently cannot analyze protocols 2.0.0-alpha.0 internal release build.

@y3rsh y3rsh self-assigned this Jul 10, 2024
@y3rsh y3rsh requested a review from a team as a code owner July 10, 2024 21:10
@y3rsh y3rsh requested review from TamarZanzouri and removed request for a team July 10, 2024 21:10
@y3rsh y3rsh changed the title Rqa 2830 mac python package RQA-2830 App cannot analyze due to mismatched build architecture of dmg and bundled python Jul 10, 2024
@y3rsh y3rsh requested a review from shlokamin July 10, 2024 21:12
@sfoster1 sfoster1 closed this Jul 10, 2024
@sfoster1 sfoster1 reopened this Jul 10, 2024
@sfoster1
Copy link
Member

Sorry 😬 misclick

Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

This looks like a thing that we should do, but I do not think it will fix the problem.
Summary:

  • We should make this change because I don't think older intel macs can run arm64 applications, so I'm glad we caught this
  • I do not anticipate that this change will fix the specific problem causing analysis failures, which is pydantic's native solib being the wrong architecture relative to python

The traceback in the bug is about loading the solib for pydantic's native-compiled parts.
Investigating the 2.0.0-alpha.0 build, we have

  • The core executable of the app is arm64 (fine? Unclear whether this would get translated on older intel macs, but definitely fine on my m2)
  • The python executable is x64 (fine, and expected since we install that ourselves) and so are its core dylibs
  • The native python deps are arm64 (this is now bad, because rosetta won't save a dlopen() to the wrong arch)

Compare to 1.5.0, where everything involved is x64 and works swimmingly.

Changing the arch here to arm64 will cause the electron install, and thus the wrapping application and supporting solibs, to be x64 instead of arm64. This is helpful because older machines can now run it again. However, we actually install python and friends ourselves in a script; that means that even though we built the application for arm64, we continued to install x64 python (because we hardcoded that as both the main option and the fallback).

Installing x64 python in an arm64 build is fine and not the cause of the problem, because the app invokes python as an executable and thus it will be seamlessly emulated through rosetta2.

The problem is that after we install python in that script, we invoke the host python - not the one we just downloaded, the one that's installed on the system - to install deps. That will use the system's architecture when it comes to native code, so it downloads arm64 deps, and while rosetta handles executable invocation it won't natively translate a solib.

So this PR as it stands will have

  • an x64 entrypoint executable and core solibs (good, but sadly unrelated)
  • an x64 python and python core solibs (good)
  • an arm64 pydantic extension solib (bad)
    and will still have the same problem.

In other words, we also need to explicitly make sure to download the right arch. One way to do that would be to figure something out with github but also... we just downloaded a python that has the correct architecture! So we should add this patch to the pr:

diff --git a/app-shell/scripts/before-pack.js b/app-shell/scripts/before-pack.js
index 6e96609786..bb5e2d4548 100644
--- a/app-shell/scripts/before-pack.js
+++ b/app-shell/scripts/before-pack.js
@@ -118,7 +118,16 @@ module.exports = function beforeBuild(context) {

       // TODO(mc, 2022-05-16): explore virtualenvs for a more reliable
       // implementation of this install
-      return execa(HOST_PYTHON, [
+      console.log(
+        `Installing python native deps using ${path.join(
+          PYTHON_DESTINATION,
+          'python3.10'
+        )}`
+      )
+      const invokablePython = platformName.includes('darwin')
+        ? path.join(PYTHON_DESTINATION, 'python/bin/python3.10')
+        : HOST_PYTHON
+      return execa(invokablePython, [
         '-m',
         'pip',
         'install',

which will download a python and then use it to install stuff.

This works in my dev testing on my machine (though this is really just by inspecting the architecture of the downloaded artifacts, since you can't actually build a full image without signing keys). It'll definitely be needed here.

@y3rsh
Copy link
Member Author

y3rsh commented Jul 11, 2024

@sfoster1 totally follow. I tried to use the downloaded python but I invoked it wrong and took it out and opened up the PR to get help. Thank you.

Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

Approving to unblock this but I contributed that second patch so somebody else should also review

@y3rsh
Copy link
Member Author

y3rsh commented Jul 11, 2024

closing in favor of #15624

@y3rsh y3rsh closed this Jul 11, 2024
y3rsh added a commit that referenced this pull request Jul 11, 2024
…ip install (#15624)

## Replaces #15618 with PR against branch name ending in
`app-build-internal` to trigger a build

## Overview

- <https://opentrons.atlassian.net/browse/RQA-2830>
- `macos-11` GitHub runner was intel and now we use macos-latest that is
arm
[doc](https://docs.github.com/en/actions/using-github-hosted-runners/about-github-hosted-runners/about-github-hosted-runners#standard-github-hosted-runners-for-public-repositories)
- specify --x64 argument to electron-builder
- ~~per
[doc](https://github.com/actions/setup-python?tab=readme-ov-file#supported-architectures)
x64 python should be installed so the pip install will be the right
platform. I'm not confident in this.~~
- see comment in #15618
- we need to use the python we download on mac to do the pip install so
the installed packages match the platform correctly

## Test Plan

- [x] build locally and see that it works
- [x] watch CI and install the built package
- [x] validate that once the package dmg is x64 that analysis works
(python has packages with the right arch)

# Review requests

- This the right approach?

# Risk assessment

High because our .dmg currently cannot analyze protocols 2.0.0-alpha.0
internal release build.
y3rsh added a commit that referenced this pull request Jul 22, 2024
…ip install (#15624)

## Replaces #15618 with PR against branch name ending in
`app-build-internal` to trigger a build

## Overview

- <https://opentrons.atlassian.net/browse/RQA-2830>
- `macos-11` GitHub runner was intel and now we use macos-latest that is
arm
[doc](https://docs.github.com/en/actions/using-github-hosted-runners/about-github-hosted-runners/about-github-hosted-runners#standard-github-hosted-runners-for-public-repositories)
- specify --x64 argument to electron-builder
- ~~per
[doc](https://github.com/actions/setup-python?tab=readme-ov-file#supported-architectures)
x64 python should be installed so the pip install will be the right
platform. I'm not confident in this.~~
- see comment in #15618
- we need to use the python we download on mac to do the pip install so
the installed packages match the platform correctly

## Test Plan

- [x] build locally and see that it works
- [x] watch CI and install the built package
- [x] validate that once the package dmg is x64 that analysis works
(python has packages with the right arch)

# Review requests

- This the right approach?

# Risk assessment

High because our .dmg currently cannot analyze protocols 2.0.0-alpha.0
internal release build.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants