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

Add support to build AppImages on ARM #1564

Merged
merged 2 commits into from Dec 8, 2023

Conversation

rmartin16
Copy link
Member

@rmartin16 rmartin16 commented Dec 6, 2023

Changes

  • Add support for packaging AppImages for ARM
  • Bumps default manylinux image from manylinux2014 to manylinux_2_28
    • The linuxdeploy tool for ARM requires at least glibc 2.25
    • Python users are around 90% compatible with glibc 2.28

Notes

  • arm64 support is available for both docker and non-docker builds
  • It becomes more awkward for armv7 since manylinux doesn't publish an image for that arch
    • I added a warning during the create process but I didn't want to prevent using docker altogether for 32-bit arm since users could use whatever docker image they want
  • The linuxdeploy support for ARM requires armhf....so, armv6 support is excluded

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

@rmartin16 rmartin16 force-pushed the arm-appimage branch 2 times, most recently from 220dd47 to 3938e7c Compare December 7, 2023 16:19
@rmartin16
Copy link
Member Author

So, when only considering aarch64 support, this is all quite trivial...however, 32bit arm does throw in a few wrenches. That said, the changes and risk of fallout is all contained to AppImage/linuxdeploy support. 32bit arm is further complicated by the fact it requires a custom support package since Standalone Python doesn't publish them. The QT plugin only supports i386 and x86_64...but it errors out rather gracefully on the download error....so, I didn't change anything there.

I'm not able to test on Apple Silicon....but in theory anyway it should work in the aarch64 manylinux image.

@rmartin16 rmartin16 marked this pull request as ready for review December 7, 2023 17:11
@rmartin16
Copy link
Member Author

Also, for posterity....manylinux doesn't provide a i686 image for manylinux_2_28....but building the AppImage does still work without docker.

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

All looks fairly straightforward; a couple of suggestions and one clarification request.

@@ -121,7 +121,7 @@ def pyproject_table_linux_system_arch(self):

def pyproject_table_linux_appimage(self):
return """
manylinux = "manylinux2014"
manylinux = "manylinux_2_28"
Copy link
Member

Choose a reason for hiding this comment

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

We should flag this change with a removal release note (in addition to the feature), as it's a notable change to defaults.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was on the fence about whether this should be a removal note since it will only affect new projects. Although, if someone familiar with the old behavior created a wholly new project, they very well could be surprised then.

"""The architecture defined (and supported) by linuxdeploy for AppImages."""
system_arch = tools.host_arch

# use 32bit linuxdeploy if using 32bit Python on 64bit hardware
Copy link
Member

Choose a reason for hiding this comment

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

It took me a moment to catch on what was going here. My initial read saw this as x86_64->i686->i386 always, not just on the "32 bit python" case. That might just be me needing a larger cup of coffee, but if you can think of a way to beef up this comment so it's more obvious, it might help.

manylinux_arch = "unknown"
self.logger.warning(
f"There is not a manylinux base image for {LinuxDeploy.arch(self.tools)}"
)
Copy link
Member

Choose a reason for hiding this comment

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

Why set the arch as "unknown" and printing a warning, so the next step fails (implicitly) on the Docker image lookup, rather than straight up failing on the arch lookup?

Copy link
Member Author

Choose a reason for hiding this comment

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

This one is a bit annoying....and I didn't want to introduce a bunch more changes to accommodate this better.

First, this code runs even if someone passes --no-docker. So, we can't error here if someone isn't even trying to use Docker. But....if this code doesn't run and they don't pass --no-docker next time, then the Dockerfile will definitely be hosed. (This may be best resolved by telling the user to run briefcase create linux appimage again since they want to use Docker now....but then we'd need to track they didn't use it the first time.)

Second, if someone isn't using the Briefcase template for AppImage, then the Dockerfile probably won't care about which specific manylinux image this logic determines anyway. So, I suppose we could assess the proper architecture for the name of the manylinux image after the code checks if app.manylinux exists.....but that just kinda moves the error condition around a bit and doesn't really change the underlying situation.

I don't think any of this is ideal....but I was mostly trying to avoid refactoring a bunch of this right now. Although, this really only presents because this is also introducing support for 32bit arm. If we only support aarch64, this complication (and others) goes away.

This is the balance I landed on given all this....open to other balancing points.

Copy link
Member

Choose a reason for hiding this comment

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

Ah - I hadn't considered the --no-docker case. That makes sense; and I think it's a reasonable compromise under the circumstances. My only thought is whether it might be better to use the actual architecture, rather than "unknown" (i.e., still raise the warning as you are now, but generate manylinux_2_28_armv7h as the manylinux base). That way, you'll still get an explicit pre-warning, but the Dockerfile will be in a state that won't look "wrong" - and when docker fails, the error will be of the form "manylinux_2_28_armv7h does not exist", rather than "manylinux_2_28_unknown does not exist". Both are equally true, but the former is a little more explicit about what doesn't exist... and it might exist at some point in the future.... (I doubt it, but it's at least possible)

except KeyError:
manylinux_arch = "unknown"
self.logger.warning(
f"There is not a manylinux base image for {LinuxDeploy.arch(self.tools)}"
Copy link
Member

Choose a reason for hiding this comment

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

Slightly awkward wording; suggest:

Suggested change
f"There is not a manylinux base image for {LinuxDeploy.arch(self.tools)}"
f"There is no manylinux base image for {LinuxDeploy.arch(self.tools)}"

@rmartin16
Copy link
Member Author

more iOS simulator fun:

[verifyapp] Starting app on an iPhone SE (3rd generation) running iOS 16.2 (device UDID 1A0F79A9-B187-44AF-9000-8558A7179437)
Booting simulator... done
The application /Applications/Xcode_14.2.app/Contents/Developer/Applications/Simulator.app cannot be opened for an unexpected reason, error=Error Domain=NSOSStatusErrorDomain Code=-600 "procNotFound: no eligible process with specified descriptor" UserInfo={_LSLine=387, _LSFunction=_LSAnnotateAndSendAppleEventWithOptions}
Opening simulator...

Unable to open iPhone SE (3rd generation) simulator running 16.2

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

👍

@freakboy3742 freakboy3742 merged commit 230f3c1 into beeware:main Dec 8, 2023
44 checks passed
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.

None yet

2 participants