-
-
Notifications
You must be signed in to change notification settings - Fork 366
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
Implement architecture and bitness checks #1360
Changes from 5 commits
95bf95a
a093588
4259a3d
2caa568
3dd2018
1e6184c
2ef9191
a7abd96
e147b56
a081900
3fada7a
77cf5df
5f184d3
d2ed115
f636e6e
5aedb7c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Support for less common environments, such as Linux on ARM, is improved and error messages for unsupported platforms are more accurate. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,7 @@ | |
BriefcaseCommandError, | ||
CorruptToolError, | ||
MissingToolError, | ||
UnsupportedHostError, | ||
) | ||
from briefcase.integrations.base import ManagedTool, Tool, ToolCache | ||
|
||
|
@@ -48,6 +49,21 @@ def download_url(self) -> str: | |
def file_path(self) -> Path: | ||
"""The folder on the local filesystem that contains the file_name.""" | ||
|
||
@classmethod | ||
def arch(cls, host_os: str, host_arch: str) -> str: | ||
# always use the x86-64 arch on macOS since the | ||
# manylinux image will always be x86-64 for macOS | ||
arch = "x86_64" if host_os == "Darwin" else host_arch | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ...is that true? The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's true in this implementation since this same code is used to determine the x86-64 manylinux image should always be used on macOS. Nonetheless, this review has included a lot of questions about building AppImages on macOS. My intention was to support building x86_64 AppImages by using the x86_64 image of manylinux on macOS irrespective of hardware. Therefore, while the aarch64 image of manylinux works on Apple Silicon, you can't then build an x86_64 AppImage with that image....and without aarch64 support from linuxdeploy, you won't be able to build any AppImage. Does that help clarify our difference of position? I'm not sure exactly where we aren't aligned. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I think that clarifies things. I think I was reading some of this PR as an attempt at "make all the architectures work everywhere"; keeping it to "native architecture only" definitely make sense as a scoping exercise, and avoids a bunch of gnarly compatibility issues. That said - as a longer term issue, I'd be interested in whether running x86 docker on M1 Mac images is viable for Linux System and Windows - mostly because it would make my personal testing life a lot simpler. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That's is definitely in scope currently for #1392 😃 |
||
try: | ||
return { | ||
"x86_64": "x86_64", | ||
"i686": "i386", | ||
}[arch] | ||
freakboy3742 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
except KeyError as e: | ||
raise UnsupportedHostError( | ||
f"Linux AppImages cannot be built on {host_arch}." | ||
) from e | ||
|
||
def exists(self) -> bool: | ||
return (self.file_path / self.file_name).is_file() | ||
|
||
|
@@ -208,7 +224,7 @@ class LinuxDeployQtPlugin(LinuxDeployPluginBase, ManagedTool): | |
|
||
@property | ||
def file_name(self) -> str: | ||
return f"linuxdeploy-plugin-qt-{self.tools.host_arch}.AppImage" | ||
return f"linuxdeploy-plugin-qt-{self.arch(self.tools.host_os, self.tools.host_arch)}.AppImage" | ||
|
||
@property | ||
def download_url(self) -> str: | ||
|
@@ -319,7 +335,7 @@ def file_path(self) -> Path: | |
|
||
@property | ||
def file_name(self) -> str: | ||
return f"linuxdeploy-{self.tools.host_arch}.AppImage" | ||
return f"linuxdeploy-{self.arch(self.tools.host_os, self.tools.host_arch)}.AppImage" | ||
|
||
@property | ||
def download_url(self) -> str: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -44,14 +44,20 @@ def project_path(self, app): | |
|
||
def binary_name(self, app): | ||
safe_name = app.formal_name.replace(" ", "_") | ||
return f"{safe_name}-{app.version}-{self.tools.host_arch}.AppImage" | ||
arch = LinuxDeploy.arch(self.tools.host_os, self.tools.host_arch) | ||
return f"{safe_name}-{app.version}-{arch}.AppImage" | ||
|
||
def binary_path(self, app): | ||
return self.bundle_path(app) / self.binary_name(app) | ||
|
||
def distribution_path(self, app): | ||
return self.dist_path / self.binary_name(app) | ||
|
||
def verify_tools(self): | ||
"""Verify the AppImage LinuxDeploy tool and its plugins exist.""" | ||
super().verify_tools() | ||
LinuxDeploy.verify(tools=self.tools) | ||
Comment on lines
+56
to
+59
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Verify LinuxDeploy before building an entire Docker image and then informing the user that LinuxDeploy can't run on ARM. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Completely agreed that we should verify LinuxDeploy before building the Docker image. However, this isn't working for me on my M1. When I run There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is something I've discovered in #1392; if you don't specify an explicit platform to use and the image only supports a platform that's different than the host platform, you get this warning. Nonetheless, (and ignoring the completely corrupted I haven't fully processed that issue....so, I look more at this later. |
||
|
||
def add_options(self, parser): | ||
super().add_options(parser) | ||
parser.add_argument( | ||
|
@@ -154,7 +160,11 @@ def output_format_template_context(self, app: AppConfig): | |
# Add the manylinux tag to the template context. | ||
try: | ||
tag = getattr(app, "manylinux_image_tag", "latest") | ||
context["manylinux_image"] = f"{app.manylinux}_{self.tools.host_arch}:{tag}" | ||
manylinux_arch = { | ||
"x86_64": "x86_64", | ||
"i386": "i686", | ||
freakboy3742 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}[LinuxDeploy.arch(self.tools.host_os, self.tools.host_arch)] | ||
context["manylinux_image"] = f"{app.manylinux}_{manylinux_arch}:{tag}" | ||
if app.manylinux in {"manylinux1", "manylinux2010", "manylinux2014"}: | ||
context["vendor_base"] = "centos" | ||
elif app.manylinux == "manylinux_2_24": | ||
|
@@ -213,11 +223,6 @@ class LinuxAppImageOpenCommand(LinuxAppImageMostlyPassiveMixin, DockerOpenComman | |
class LinuxAppImageBuildCommand(LinuxAppImageMixin, BuildCommand): | ||
description = "Build a Linux AppImage." | ||
|
||
def verify_tools(self): | ||
"""Verify the AppImage linuxdeploy tool and plugins exist.""" | ||
super().verify_tools() | ||
LinuxDeploy.verify(tools=self.tools) | ||
|
||
def build_app(self, app: AppConfig, **kwargs): # pragma: no-cover-if-is-windows | ||
"""Build an application. | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a huge thing - but any reason to merge
is_32bit
andplatform
, rather than keeping them separate until needed for output?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I was getting frustrated with linters telling me a line was too long. I should figure out a different solution though.