feat(CI): Keep local and Github CI test in sync with cargo xtask#23414
feat(CI): Keep local and Github CI test in sync with cargo xtask#234142010YOUY01 wants to merge 1 commit into
cargo xtask#23414Conversation
| "--release-source", | ||
| "rust-dist", |
There was a problem hiding this comment.
These two lines added should be the only changed command (I found the default option fails on my local setting)
|
cc who might be interested, @alamb @comphead @kumarUjjawal @blaginin |
comphead
left a comment
There was a problem hiding this comment.
Thanks @2010YOUY01 does that mean CI changes should be reflected in xtask and vice versa?
WDYT about https://github.com/nektos/act to try run DataFusion CI locally, although I heard not all things possible with this tool
| .iter() | ||
| .find(|job| job.name == VERIFY_CLEAN_JOB_NAME) | ||
| .unwrap_or_else(|| panic!("missing `{VERIFY_CLEAN_JOB_NAME}` job")); | ||
| let mut jobs = vec![verify_clean]; |
There was a problem hiding this comment.
fails immediately if we have there are any uncommitted changes before it runs any CI check. Was this intentional?
There was a problem hiding this comment.
Yes, this is intentional, now the whole workflow requires a clean local git repo to run (no uncommitted changes), though I think there is room to improve it later.
To better simulate GitHub CI, we should try to reproduce the actual commands it runs as closely as possible.
Currently, GitHub CI runs several verify-clean checks between tests to ensure that no generated or modified files are left behind after each test, using git diff.
So for the local reproducer to behave the same way, we need to ensure there are no uncommitted changes before running the full workflow.
| format!("failed to create {}: {error}", tpch_data.display()) | ||
| })?; | ||
|
|
||
| self.command("git") |
There was a problem hiding this comment.
if the step fails partway through and I re-run, git clone immediately fails with "destination path already exists". I'd have to manually rm -rf tpch-dbgen before retrying.
There was a problem hiding this comment.
It keeps the existing command, we could fix it later. See #23414 (comment)
| .env("RUSTFLAGS", rustflags) | ||
| .run()?; | ||
|
|
||
| let chrome_driver = format!( |
There was a problem hiding this comment.
This results in wasm-pack being called with --chromedriver /chromedriver , which fails with a confusing "not found" error. The run_rust_sqllogictest function handles the missing-env-var case much better by returning an explicit error, we can use the same here
There was a problem hiding this comment.
Similarly, #23414 (comment)
I found this job also a bit tricky to setup locally on my Mac
| let result = runner(self, &args); | ||
| job_timing.add_step_timing(step, start.elapsed().unwrap_or_default()); | ||
|
|
||
| if let Err(error) = result |
There was a problem hiding this comment.
we should consider building the full message in a single map_err rather than chaining, there are several instances
Yes, exactly. That's the end goal, and this PR is the initial step with a limited scope.
Good to know about
Overall I think |
| use crate::ci_workflows::WorkflowInfo; | ||
|
|
||
| const VERIFY_CLEAN_JOB_NAME: &str = "verify-clean"; | ||
| const SKIPPED_JOBS: &[&str] = &[ |
There was a problem hiding this comment.
These jobs are not supported by the local runner yet. I found that they need some modifications to run on macOS.
This PR does not change any commands, to make the review easier.
Summary
Part of #21048
For easier review, start with this PR writeup, then read the top comments in
.github/workflows/rust.yml, and then follow along with the code.This PR introduces an easy way to reproduce CI checks locally.
To reproduce
.github/workflows/rust.yml:Example output:
To reproduce single job or step similarly,
Background
The existing GitHub CI follows a 3-layer structure:
cargo fmtcheck-fmt.github/workflows/rust.ymlFor example:
This PR mirrors that structure in
xtask, so the same CI units can be used by both GitHub Actions and local runs.Motivation
Making CI easy to reproduce locally improves developer experience.
cargo xtask ci job <job>.Possible future extensions:
Detect changed Rust files, then only run unit tests and clippy on crates affected by the change, plus relevant integration tests.
For documentation-only changes, only run formatting, typo checks, docs checks, and other lightweight checks.
Implementation
The implementation adds reusable units for atomic checks, such as
cargo fmt. In GitHub Actions terminology, these are "steps".The key idea is:
cargo xtask ci ...orchestrates the same checks locally.There are two viable ways to organize these atomic checks:
cargo xtask, for examplecargo xtask ci step fmt./check_fmt.shI previously tried the shell script approach for
dev.yml:https://github.com/apache/datafusion/blob/main/dev/rust_lint.sh
For the current CI complexity, I found
cargo xtaskeasier to keep organized. I suggest we continue with this approach and later migrate the existing scripts to usextaskas well.Scope of this PR
This PR:
xtaskframework.rust.ymltoxtaskrunners. The commands are unchanged, except for the small difference noted below.cargo xtask ci workflow rust.Notes:
Testing
Individual 'step's will be tested by CI.
For workflow/jobs, I have tested locally on MacOS. I think we can let CI to test some fast workflows in the future.