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

Implement cross compile to validate i686 in addition to x86_64 tests. #247

Merged
merged 5 commits into from
Dec 20, 2024

Conversation

simonrupf
Copy link
Contributor

@simonrupf simonrupf commented Dec 14, 2024

As per discussion in #245 (comment) this change experiments with adding a matrix build for the unit tests. For now it only adds i686 to the existing x86_64 to catch out 32bit related differences. It seems to run in a similar time frame as the prior runs before this change, 1min to 1min 15s.

Changes in detail:

  • adds the same matrix job style with cross as in the release.yml to the rust.yml workflow, but only for x86_64 & i686 targets
  • to avoid taking longer (due to cross install) in the i686 case, I disabled the IMHO redundant clippy and check steps on the cross enabled builds
  • bumps all checkout actions to v4 (noticed while comparing the workflows, can remove this again if not wished for)

@simonrupf simonrupf force-pushed the 32bit-build branch 3 times, most recently from 345921f to e5c3a67 Compare December 14, 2024 07:47
@simonrupf simonrupf marked this pull request as ready for review December 14, 2024 07:59
@01mf02
Copy link
Owner

01mf02 commented Dec 16, 2024

Thanks a lot for your PR, @simonrupf!

A few ideas:

  • I just came across a GitHub action that seems to support installing multiple targets (without cross). I would prefer such a solution over cross (because the latter requires running two different workflows, namely one for installing the Rust toolchain and one for installing cross). At a later point, it could even be possible to replace cross with this solution also in release.yml, but I'm not sure about that ... If you could test this already just for the checking workflow, I would be very grateful.
  • I like your idea of running only tests on different architectures. I would make this idea clearer by making a separate workflow, such as test.yml for running tests, and check.yml, for running cargo check and cargo clippy (using different platforms). That would also make the weird name rust.yml obsolete.

@simonrupf
Copy link
Contributor Author

Sure thing, lets use this opportunity to experiment with a few things.

@simonrupf simonrupf force-pushed the 32bit-build branch 2 times, most recently from df3dc77 to 19a3207 Compare December 16, 2024 18:24
@simonrupf
Copy link
Contributor Author

Ok, so using actions-rust-lang/setup-rust-toolchain is not enough, when using an alternative target. One still needs other tools, such as libgcc that it links to, in the target architecture. I used taiki-e/setup-cross-toolchain-action to install whats necessary, an action I have some prior experience using for that.

Let me know if that is an acceptable use or if you know of any other actions we could try.

@01mf02
Copy link
Owner

01mf02 commented Dec 17, 2024

Ok, so using actions-rust-lang/setup-rust-toolchain is not enough, when using an alternative target. One still needs other tools, such as libgcc that it links to, in the target architecture. I used taiki-e/setup-cross-toolchain-action to install whats necessary, an action I have some prior experience using for that.

Let me know if that is an acceptable use or if you know of any other actions we could try.

Yes, this looks very good! Thanks a lot so far!

Do you think, by the way, that the method that you used in this workflow can be applied to the release workflow as well? Some time ago, a few targets (such as arm-unknown-linux-musleabihf) started to fail building, and I have not been able to re-enable those. If you are motivated to try getting these back to work, then this would be much appreciated.

@simonrupf simonrupf force-pushed the 32bit-build branch 4 times, most recently from 4d01edf to b5a4cf0 Compare December 17, 2024 17:18
For testing purposes this workflow triggers on every commit, but got defanged not to use secrets or publish.
@simonrupf
Copy link
Contributor Author

I created this in a two step process, to be able to test the workflow without triggering a release. Last clean run of that test workflow can be found at:
https://github.com/01mf02/jaq/actions/runs/12378092195

The windows gnu build still fails, due to a tool or library with the wrong version, details in the comment above the disabled target and error can be found in:
https://github.com/01mf02/jaq/actions/runs/12377602714/job/34547502612#step:5:179

One of the arm targets seems no longer supported, the error indicates:

target 'arm-unknown-linux-gnueabihf' not yet supported; consider using armv7-unknown-linux-gnueabihf for testing armhf or arm-unknown-linux-gnueabi for testing armv6

So I added those two instead and they build successfully.

@01mf02
Copy link
Owner

01mf02 commented Dec 20, 2024

This is very good work, @simonrupf! Thanks a lot for your dedication!

@01mf02 01mf02 merged commit 36c40f1 into 01mf02:main Dec 20, 2024
3 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.

2 participants