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

fix: compilation with default-features = false #74

Merged
merged 1 commit into from
Apr 22, 2024

Conversation

DaniPopes
Copy link
Contributor

@DaniPopes DaniPopes commented Apr 9, 2024

Fix compilation with default-features = false, meaning without the k256 feature, also testing it in CI.

I took the opportunity to also improve the CI with more checks, I adapted it from what we have at alloy-rs/core.

Copy link
Member

@AgeManning AgeManning left a comment

Choose a reason for hiding this comment

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

Hey thanks!

Cleaning up the code it looks good!

In regards to the CI, the cargo hack command seems cool and useful.

In Lighthouse, we somewhat recently overhauled our CI for github runners in attempting to minimize CI time and resource consumption.

A few things we found useful were

  • Grouping into a single job
  • Adding caching
  • cargo nexttest for testing

I also kind of liked the fact that there is a single repo we are tied to for these tests, moonrepo. We can download the binaries straight up for cargo audit and nexttext etc, rather than compiling them every CI run.

For this repo, the CI doesn't take much time, but I notice this PR undoes some of the things mentioned above. Specifically

  • We now add 3 source repos (dtolnay, taiki-e, Swatinem)
  • We ungroup some of the jobs (clippy, fmt and audit)
  • We revert back to cargo test from cargo nexttest

Are these intentional? I'm not overly concerned, just from an initial look without much research into the extra repos and types you have here it seems like a regression.

@DaniPopes
Copy link
Contributor Author

DaniPopes commented Apr 21, 2024

Yes, these are intentional.

  • Binaries are also not compiled every time, but downloaded from GH releases, see taiki-e/install-action
  • I don't think having multiple action repos matters
  • cargo test is perfectly fine on a small repo with only unit tests; same for ungrouped actions, I'd prefer seeing what exactly failed (clippy vs fmt vs build) rather than having everything in one job

Running time went from 3m cached to 40s uncached (https://github.com/sigp/enr/actions/runs/8618027272). It technically does use more resources and has more billable time but running it is free so it ultimately doesn't matter.

@AgeManning AgeManning merged commit 12b3bc7 into sigp:master Apr 22, 2024
9 checks passed
@DaniPopes DaniPopes deleted the no-k256 branch April 23, 2024 00:56
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