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

fix: support legacy arm platforms #478

Closed
wants to merge 8 commits into from

Conversation

moelasmar
Copy link
Contributor

Issue #, if available:
#348

Description of changes:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@moelasmar moelasmar marked this pull request as draft March 29, 2023 11:40
@moelasmar moelasmar marked this pull request as ready for review March 29, 2023 13:12
Comment on lines +195 to +197
"manylinux1_aarch64": "manylinux_2_5_aarch64",
"manylinux2010_aarch64": "manylinux_2_12_aarch64",
"manylinux2014_aarch64": "manylinux_2_17_aarch64",
Copy link
Contributor

Choose a reason for hiding this comment

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

Where did you get these mapping from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was using this PR to test if my assumption is correct or not, and I marked it as draft, but seems I made it ready by mistake .. sorry for that .. I still need to work on it, it is not finalized

@@ -103,6 +105,42 @@ def test_must_build_python_project(self):
output_files = set(os.listdir(self.artifacts_dir))
self.assertEqual(expected_files, output_files)

@skipIf(NOT_ARM, "Skip in windows tests")
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't run arm tests (not sure why), but how are we verifying this works now and will work in the future?

Also the reason states "windows". I assume that is a copy paste error.

{
"inflate64",
"inflate64.libs",
"inflate64-0.1.4.dist-info",
Copy link
Contributor

Choose a reason for hiding this comment

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

Not clear to me how this works with the mappings you added. Was inflate64 the issue or was it some other package? Looking at the downloadable whls form this version, I don't see any of the aarch64 whls there.

@moelasmar moelasmar marked this pull request as draft March 30, 2023 06:52
@moelasmar moelasmar closed this May 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants