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

Get all tests passing on linux arm64 #17645

Merged
merged 3 commits into from
Nov 29, 2022
Merged

Conversation

benjyw
Copy link
Contributor

@benjyw benjyw commented Nov 26, 2022

  1. Add terraform versions for Linux arm64
  2. Switches spectral to be an NpxTool, since it has no published binary for arm64
  3. Various other test tweaks

Once we have resources to run Linux arm64 CI we can start releasing wheels for that platform.

1) Switches spectral to be an NpxTool, since there is no published
   binary for arm64.
2) Add terraform versions for Linux arm64.
@benjyw benjyw added the category:internal CI, fixes for not-yet-released features, etc. label Nov 26, 2022
@benjyw benjyw changed the title Support linux arm64 tools out of the box. Get all tests passing on linux arm64 Nov 26, 2022
@benjyw
Copy link
Contributor Author

benjyw commented Nov 26, 2022

With these changes ./pants test :: passes on Linux Arm64 with Python 3.7 and 3.8 (3.9 has a dynamic linking issue that I will look into separately).

./cargo test also passes.

"linux_arm64": "linux",
"linux_x86_64": "linux",
}
default_version = "@stoplight/spectral-cli@6.5.1"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an experimental backend, so we can break the semantics of this version string, and note that this backend will only be available in 2.15, and this change will be in place in 2.16, so the incompatibility window is narrow.

@@ -315,8 +315,10 @@ def test_coverage_html_xml_json_lcov(batched: bool) -> None:
def test_default_coverage_issues_12390() -> None:
# N.B.: This ~replicates the repo used to reproduce this issue at
# https://github.com/alexey-tereshenkov-oxb/monorepo-coverage-pants.
if platform.system() == "Darwin" and platform.processor() == "arm":
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It turns out that platform.processor() is an empty string on Linux ARM64, and in general machine() is preferred.

from pants.testutil.pants_integration_test import run_pants, setup_tmpdir

skip_on_linux_arm = pytest.mark.skipif(
platform.system() == "Linux" and platform.machine() == "aarch64",
reason="PyOxidizer is not supported on Linux ARM",
Copy link
Contributor

Choose a reason for hiding this comment

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

That seems surprising.

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 surprised too, but https://pyoxidizer.readthedocs.io/en/pyoxidizer-0.17/pyoxidizer_getting_started.html:

PyOxidizer is officially supported on the following operating systems:

- Windows x86 (32-bit)
- Windows x86_64/amd64 (64-bit)
- macOS x86_64 (Intel processors)
- macOS aarch64 (ARM/Apple processors)
- Linux i686 (32-bit)
- Linux x86_64 (64-bit)

src/python/pants/backend/terraform/tool.py Show resolved Hide resolved
@benjyw benjyw merged commit 9e32e44 into pantsbuild:main Nov 29, 2022
@benjyw benjyw deleted the aarch64_tools branch November 29, 2022 21:52
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

w00t

@thejcannon
Copy link
Member

What's this pubring.kbx at the repo root? 🧐

@benjyw
Copy link
Contributor Author

benjyw commented Dec 2, 2022

Eeeerk, I thought I had deleted that. 🤦 That was an artifact of connecting to the temporary EC2 instance I was using for testing. I have deleted the underlying instance and key pairs, and this is a public key box, so not a security issue. Will delete these from the repo.

@benjyw
Copy link
Contributor Author

benjyw commented Dec 3, 2022

#17707

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:internal CI, fixes for not-yet-released features, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants