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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rust: Add no_std CI #1648

Merged
merged 8 commits into from
Sep 5, 2023
Merged

Rust: Add no_std CI #1648

merged 8 commits into from
Sep 5, 2023

Conversation

pinkforest
Copy link
Contributor

@pinkforest pinkforest commented Sep 4, 2023

Adds CI for no_std on a target that does not have it

Prevents #1645

(The current one doesn't use no_std target and may miss it - this is the same workflow we use in curve25519-dalek)

EDIT: The workflow curently in curve25519-dalek is no_std.yml

(I will send further PR's to make the workflows compact + faster separately after this - I see some low hanging fruit 馃崜 )

Copy link
Collaborator

@JasonGross JasonGross left a comment

Choose a reason for hiding this comment

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

Could you add this as a separate job in the rust.yml file? I'd rather have just one file for all the Rust tests

.github/workflows/no_std.yml Outdated Show resolved Hide resolved
runs-on: ubuntu-latest

concurrency:
group: ${{ github.workflow }}-${{ github.head_ref || github.run_id }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we want

Suggested change
group: ${{ github.workflow }}-${{ github.head_ref || github.run_id }}
group: ${{ github.workflow }}-${{ github.head_ref || github.run_id }}-${{ github.job }}

on both this and the one above, or else the jobs are going to cancel each other

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

- name: curve25519-dalek test
run: etc/ci/test-fiat-rust-curve25519-dalek.sh
run: (cd .. ; etc/ci/test-fiat-rust-curve25519-dalek.sh)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you instead make the scripts robust to cwd changes by sticking in a line like

cd "$(dirname "$0")/../.." || exit 7

in the script itself?

Copy link
Contributor Author

@pinkforest pinkforest Sep 4, 2023

Choose a reason for hiding this comment

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

I recommend to do that in a follow-up PR where I will remove those shellscripts together and it will be natively github action - or a bit more idiomatic w/o shellscripts

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, sounds good. In that case I think you'll need to fix the cd ..; ../etc/... that's a couple lines down though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh thanks fixed 馃挏

@JasonGross
Copy link
Collaborator

this is the same workflow we use in curve25519-dalek

I don't see the taiki-e/install-action@cargo-hack in that link, what's up with this?

@pinkforest
Copy link
Contributor Author

pinkforest commented Sep 4, 2023

jobs:
test-rust:

runs-on: ubuntu-latest

concurrency:
group: ${{ github.workflow }}-${{ github.head_ref || github.run_id }}
group: ${{ github.workflow }}-${{ github.head_ref || github.run_id }}-${{ github.job }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, this doesn't work the way I hoped, and still cancels the job
Let's try

Suggested change
group: ${{ github.workflow }}-${{ github.head_ref || github.run_id }}-${{ github.job }}
group: ${{ github.workflow }}-${{ github.head_ref || github.run_id }}-${{ job.name }}

Or else just manually disambiguate them?

Copy link
Contributor Author

@pinkforest pinkforest Sep 4, 2023

Choose a reason for hiding this comment

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

I think it needs to be at root level

- name: no_std / no feat fiat-crypto
run: cargo build -p fiat-crypto --target thumbv7em-none-eabi --release --no-default-features
- name: no_std / cargo hack fiat-crypto
run: cargo hack build -p fiat-crypto --target thumbv7em-none-eabi --release --each-feature --exclude-features default,std
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this supposed to complete in 0.00s?
Run cargo build -p fiat-crypto --target thumbv7em-none-eabi --release --no-default-features
Compiling fiat-crypto v0.2.2 (/home/runner/work/fiat-crypto/fiat-crypto/fiat-rust)
Finished release [optimized] target(s) in 6.75s
0s
Run cargo hack build -p fiat-crypto --target thumbv7em-none-eabi --release --each-feature --exclude-features default,std
cargo hack build -p fiat-crypto --target thumbv7em-none-eabi --release --each-feature --exclude-features default,std
shell: /usr/bin/bash -e {0}
env:
CARGO_INCREMENTAL: 0
CARGO_TERM_COLOR: always
info: running cargo build --release --target thumbv7em-none-eabi --no-default-features on fiat-crypto (1/1)
Finished release [optimized] target(s) in 0.00s

Copy link
Contributor Author

@pinkforest pinkforest Sep 4, 2023

Choose a reason for hiding this comment

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

Yes because it only builds it and it uses incremental compilation - it will take more when there are features

Copy link
Collaborator

@JasonGross JasonGross left a comment

Choose a reason for hiding this comment

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

Thank you!

@JasonGross JasonGross enabled auto-merge (squash) September 4, 2023 19:05
@JasonGross JasonGross merged commit 217951e into mit-plv:master Sep 5, 2023
14 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.

None yet

2 participants