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

Update CI Rust checks #57

Merged
merged 4 commits into from
Nov 16, 2023
Merged

Update CI Rust checks #57

merged 4 commits into from
Nov 16, 2023

Conversation

eagr
Copy link
Contributor

@eagr eagr commented Nov 12, 2023

helps #19

summary

  • use dtolnay/rust-toolchain instead
  • use nightly for both fmt and clippy, for the latest features
  • clippy -- --deny warnings (let me know if this isn’t what you want)
  • use matrix for testing (easier to add more targets)

I combined fmt and clippy into one job so that we don’t have to repeat the checkout-install ceremony. Also coz by definition linting does address stylistic errors. :)) But if you don’t like it, I could split them, no problem.

Curious, is there a reason why we’re using ubuntu-20.04 instead of ubuntu-latest?

@liamaharon
Copy link
Contributor

Curious, is there a reason why we’re using ubuntu-20.04 instead of ubuntu-latest?

No reason in particular. We could probably bump to 22.04, but I don't think it's so consequential.

@liamaharon liamaharon added the enhancement New feature or request label Nov 14, 2023
@eagr eagr marked this pull request as draft November 14, 2023 07:54
@eagr
Copy link
Contributor Author

eagr commented Nov 14, 2023

Interesting, normally dev build should be faster, but not the other way around. Any idea why?

@liamaharon
Copy link
Contributor

Interesting, normally dev build should be faster, but not the other way around. Any idea why?

Weird GH Action env 🤷

Copy link
Contributor

@liamaharon liamaharon left a comment

Choose a reason for hiding this comment

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

Thanks for this!

@eagr
Copy link
Contributor Author

eagr commented Nov 14, 2023

Just a wild guess. Maybe, unlike the release build, somehow the dev build artifacts are not properly cached in the environment, so it needs to build dependencies every time.

Now that the cache is in place, future test runs should hopefully see some improvements. But I made linting slower, probably a caching problem. I'll fix that.

@eagr
Copy link
Contributor Author

eagr commented Nov 14, 2023

let block_hash = common::block_hash(block_number, &ws_url).await.unwrap();

Thread 'create_snapshot_works' panicked at 'called `Result::unwrap()` on an `Err` value: "`WsClientBuilder` failed to build:
Transport(Error when opening the TCP socket: Connection refused (os error 111))"',
/home/runner/.cargo/git/checkouts/polkadot-sdk-cff69157b985ed76/cfb2925/substrate/test-utils/cli/src/lib.rs:263

The code panicks with dev build in CI, the workflow stops right there, before the tooling has the chance to save the artifacts. That's why the build is always slow. Any idea why it would break on dev build (but not on release)?

@liamaharon
Copy link
Contributor

liamaharon commented Nov 14, 2023

Any idea why it would break on dev build (but not on release)?

Maybe the debug build is too slow on the runner and it can't properly establish a connection.

The runner is very computationally limited and the test pretty heavy, it spins up an entire node.

@eagr
Copy link
Contributor Author

eagr commented Nov 14, 2023

Updates:

  • Pin nightly to make caching more effective
  • Tag lint cache to differentiate it (it's not entirely necessary, just to be safe)
  • Remove the cleansing step as it's no longer needed now, which could save another 150s. We'd have a 10G margin (after the test), should be safe, right? But in case I'm wrong, it's easy to add it back.

The jobs now complete in ~16min, seems reasonable.

image

@eagr eagr requested a review from liamaharon November 14, 2023 16:43
@eagr eagr marked this pull request as ready for review November 14, 2023 16:44
@eagr
Copy link
Contributor Author

eagr commented Nov 14, 2023

I got an idea for the test. Instead of sleeping for 90s, how about we try connecting every X seconds, but with a deadline. May make the tests less fragile. Also may be able to save us a bit more time. Do you think it makes sense?

@simonsso
Copy link
Contributor

The runner is very computationally limited and the test pretty heavy, it spins up an entire node.

@liamaharon do you have access to some faster build nodes like the ubuntu-latest-4-cores or ubuntu-latest-8-cores in this project?

@liamaharon
Copy link
Contributor

liamaharon commented Nov 16, 2023

I got an idea for the test. Instead of sleeping for 90s, how about we try connecting every X seconds, but with a deadline. May make the tests less fragile. Also may be able to save us a bit more time. Do you think it makes sense?

This has been on my radar for a while I just haven't gotten around to it. Would be great to see this improvement in a new PR :)

@liamaharon do you have access to some faster build nodes like the ubuntu-latest-4-cores or ubuntu-latest-8-cores in this project?

We could probably, but I'm quite happy with the numbers in this PR I don't think it is necessary.

@liamaharon liamaharon merged commit fd2b5ef into paritytech:main Nov 16, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants