Revive CI with GH Workflows #6
Conversation
aewag
force-pushed
the
revive-ci-with-gh-workflows
branch
11 times, most recently
from
7a7494a
to
bfcea46
Compare
aewag
force-pushed
the
revive-ci-with-gh-workflows
branch
2 times, most recently
from
77f4bcc
to
65987cf
Compare
|
@newpavlov Have you had a chance to look at this PR? |
aewag
force-pushed
the
revive-ci-with-gh-workflows
branch
from
65987cf
to
9a33bfd
Compare
|
Updated because |
|
@aewag sorry, wasn't subscribed to this repo and didn't see these PRs. Thanks for submitting them! |
| - name: Install precompiled cross | ||
| run: | | ||
| export URL=$(curl -s https://api.github.com/repos/cross-rs/cross/releases/latest | \ | ||
| jq -r '.assets[] | select(.name | contains("x86_64-unknown-linux-gnu.tar.gz")) | .browser_download_url') | ||
| wget -O /tmp/binaries.tar.gz $URL | ||
| tar -C /tmp -xzf /tmp/binaries.tar.gz | ||
| mv /tmp/cross ~/.cargo/bin | ||
| shell: bash |
There was a problem hiding this comment.
Curious how much time this actually saves versus cargo install cross, which we use elsewhere:
Looking at the CI history, it takes about 1m11s
There was a problem hiding this comment.
Yep, I also thought about using cargo install cross. The benefit is not too big for the more complex CI code.
cross within my fork takes 30 - 40 seconds for a single run.
https://github.com/aewag/sponges/actions/runs/1745945800
I can push a variant using cargo install, and we can see if it performs much worse.
There was a problem hiding this comment.
Well, there seems to be a different issue than performance:
The current MSRV of the keccak crate is 1.41. But this is not sufficient to build cross [1]. So best is probably to work with the precompiled cross. I removed the WIP commit for experimenting with cargo install.
[1] https://github.com/aewag/sponges/runs/4940099815?check_suite_focus=true#step:3:170
There was a problem hiding this comment.
It's ok to bump MSRV, especially if we're making breaking changes anyway
There was a problem hiding this comment.
Isn't than the MSRV of cross the bottomline MSRV of keccak?
I just pushed a WIP with MSRV 1.46. This succeeds building cross. Interestingly prior versions fail [1], although cross MSRV is 1.42. Not really sure why this is the case.
Performance-wise it takes 2m for a single run compared to 30-40s with a precompiled binary.
[1] https://github.com/aewag/sponges/runs/4942085469?check_suite_focus=true
There was a problem hiding this comment.
Perhaps @aewag can check me on this but I think the
set-msrvpattern introduced in this PR (and also this one, which now has differing cross-install boilerplate) could potentially be extracted as a reusable workflow?
Yes, set-msrv is a candidate to be moved into a reusable workflow. For example, this would easily allow an extension to get the MSRV from the Cargo.toml instead of hardcoding it in the CI config. I think for such extensions it is nice to have common patterns at a single place.
Additionally it seems like actions-rs is unmaintained and a rather heavyweight solution for how we actually use it. Perhaps we could fork it and slim it down to our specific needs. That would eliminate dependence on third-party actions we don't control.
I suggest to install cross using a composite action. This seems to me the most lightweight approach, that is currently available. If actions-rs gets extended or cross supports better options to install, we could simply switch to that by modifying this action only.
There was a problem hiding this comment.
@newpavlov WDYT? Should we give it a try?
It seems like we could use set-msrv everywhere to have MSRV specified once per workflow.
There was a problem hiding this comment.
@tarcieri
Yeah, go ahead. We always can revert everything back if it will be too much trouble in the end.
There was a problem hiding this comment.
I made a repo here: https://github.com/RustCrypto/actions
@aewag feel free to upstream the work there! It seems like we'll be able to unify the configs in sponges and hashes
There was a problem hiding this comment.
I made a repo here: https://github.com/RustCrypto/actions
@aewag feel free to upstream the work there! It seems like we'll be able to unify the configs in
spongesandhashes
Perfect, we'll do that. 👍
aewag
force-pushed
the
revive-ci-with-gh-workflows
branch
5 times, most recently
from
5463d25
to
332e47a
Compare
aewag
force-pushed
the
revive-ci-with-gh-workflows
branch
6 times, most recently
from
a3f8e5d
to
9a33bfd
Compare
|
Thank you! |
PR revives the CI with help of the GH Workflows, similar to other crates in the RustCrypto repositories.
Tests are ran with & without
no_unrollfeature. Ascross testscurrently does not supportthumbv7em-none-eabias a target, this is not covered, but build tests are run for this target.I have not sufficient rights to trigger Github Actions in this repository. I tested this PR within my fork:
aewag#1
Further, this PR contains commits to make rustfmt and clippy happy. To skip the rustfmt on the macros, I moved them into
unrolled.rsand flagged it with the skip attribute.