Use ARM runners#321
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
konstantinoscs
left a comment
konstantinoscs
left a comment
There was a problem hiding this comment.
There are a couple of critical things we should fix here. Otherwise looks good to me
| . /tmp/venv/bin/activate | ||
| pip3 install dist/*.whl --force-reinstall | ||
| ${{ env.EXECUTABLE_NAME }} --help | ||
| manylinux: auto |
There was a problem hiding this comment.
We should put this back to 2_28. I dug a bit on what auto means and it basically says "pick the lowest compatible/default manylinux setup". The logs for the CI on this PR show that this effectively downgraded us to 2_17. We moved to 2_28 on purpose at #277 for very good reasons so we should keep this
| - uses: actions/checkout@v6 | ||
| with: | ||
| submodules: recursive | ||
| persist-credentials: false |
There was a problem hiding this comment.
We need to keep this. persist-credentials runs with true by default and we don't want it here as subsequent steps run actual code and having a GITHUB token available to them is.... not good.
Don't persist credentials
| uses: PyO3/maturin-action@v1.50.1 | ||
| with: | ||
| target: ${{ matrix.platform.target }} | ||
| manylinux: auto |
There was a problem hiding this comment.
I see you moved the other jobs to 2_28 as well. I guess because of consistency. That's ok for me but the cost we're paying is that for those architectures, we're dropping older glibc users (e.g. someone on Ubuntu 18.04).
For the arm runner we need 2_28 because of the bug we have (again, in #277).
We don't techincally need the same for x64 and we could theoretically support older glibc users but I think being consistent is better for maintenability here. So I'd say leave it, I just wanted to point it out
There was a problem hiding this comment.
Yeah I'd rather be consistent unless there's truly a need to have auto here. If it ends up causing issues for someone on an older version of Linux we can definitely revert this or pin to an older version.
2 participants
This PR updates the
linuxandmusllinuxjobs to use ARM runners directly, rather than doing cross compilation.