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.
alamb
left a comment
There was a problem hiding this comment.
Thank you @2010YOUY01 for this PR -- the idea of making the deveopment experience better is good, but I am worried about adding another tool requirement
Making CI easy to reproduce locally improves developer experience.
I do agree with this, but I also think making the development experience more bespoke (requiring yet another tool -- we already have taplo, prettier, etc) works against this purpose
For example InfluxData uses just for this purpose, and I have also used Make for this purpose. This has resulted in several times it not being possible to use standard cargo commands to run the tests (b/c the indirection layers add some magic flag)
In my opinion these tools all tend to make understanding how to do things in the repo less standard and thus put up a larger barrier to entry.
On the other hand AI tools lower this barrier, so maybe it is not as much of a problem anymore 🤔
I previously tried the shell script approach for dev.yml:
https://github.com/apache/datafusion/blob/main/dev/rust_lint.shFor the current CI complexity, I found cargo xtask easier to keep organized. I suggest we continue with this approach and later migrate the existing scripts to use xtask as well.
I agree we can't mirror CI 1:1 in shell scripts without unwarranted complexity
However I think we can get like 95% of the common failures from CI via shell scripts and that would be good enough for the repo
| # Architecture | ||
| # ------------- | ||
| # | ||
| # Each atomic CI check lives in an individual `xtask` step. The GitHub workflow |
There was a problem hiding this comment.
I have never heard of xtask -- I am sure it is awesome, but it does now add one more development dependency for developers and makes it that much harder (maybe just a little) to develop in DataFuson. (as now you need to have yet another tool installed)
| # | ||
| # (@ means no `needs` dependency.) | ||
| # | ||
| # @ -> linux-build-lib |
There was a problem hiding this comment.
Is the idea that xtask can model this type of dependency tree?
|
So in other words maybe we don't have to have 100% parity with what CI does -- instead we can focus on getting the most of the coverage via standard commands. And maybe if there is something that CI is doing that is non standard, we can try to reduce the complexity there |
|
Thank you for the detailed feedback, @alamb! I agree that the current solution adds friction for new developers. I think this approach could eventually become frictionless, but it needs more work. Let me run some experiments to see whether I can make even the initial version easier to use. Here are a few ideas:
One minor clarification: Shell scripts vs. xtask commandsTo reach our end goal, the leaf nodes (atomic checks), should be wrapped as reusable units. The main design question is whether those units should be shell scripts or I still think Let me explore ways to reduce the setup friction and see how the approach looks afterward. Additional referencesHere are a few projects I would like to examine more closely. My impression is that most projects do not provide a great developer experience for running tests locally or reproducing CI. ChatGPT suggested these three as particularly strong examples, and I would like to study their approaches and consider how we could implement something similar: |
I think you need to do cargo install cargo-xtaskI am also somewhat worried about taking on a new (admittedly dev only) dependency. For example, I took a look at https://github.com/matklad/cargo-xtask and it was last updated 9 months ago and it doesn't have an obvious long term maintenance model (e.g. bugs,. etc) beyond someone's personal project
I agree that shell scripts are not great for complex logic. What I was wondering is can we avoid complicated shell scripts by simplifying that logic (like does it need to follow all the dependency checks that the current CI does?) rather than finding a some system (like |
That It's more like a convention that is used in Rust ecosystem (e.g. cargo itself https://github.com/rust-lang/cargo/blob/master/.cargo/config.toml) to implement project-specific development commands and automation in Rust. The linked cargo-xtask repository only documents this convention, below is a quote from https://github.com/matklad/cargo-xtask
I agree we could pul such logic outside existing scripts to keep things easier to maintain. |
|
I'm trying to get a bigger picture. When we say Do we anticipate supporting 2 flows might be hard to maintain and keep them in sync and eventually the local part might be untrusted and not used as the result? 🤔 If the end goal to catch issues as early as possible or save ASF resources then contributor forks delegation would help, otherwise we may want to try to break down |
|
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.