Move integration crates in their own workspaces #69
Conversation
.github/workflows/rust.yml
Outdated
| - run: cargo fmt --all -- --check | ||
| - run: cargo clippy --all-targets -- -D warnings | ||
| components: rustfmt | ||
| - run: cd rinja && cargo fmt -- --check |
There was a problem hiding this comment.
Instead of doing it like this, why not using matrix so then we list all folders we want to go into so we can just have run: cd $FOLDER && cargo fmt -- --check?
There was a problem hiding this comment.
No need to run the checks in parallel. They are fast enough.
Even though the runners are free for open source projects, I don't think we should use more CPU time than we need to. Be eco friendly and such.
Also, we already have 17 checkmarks on each PR. At some point it gets annoying to scroll until you find the red cross on the failed test. :)
There was a problem hiding this comment.
Can't have that not in parallel?
There was a problem hiding this comment.
I don't know if there is a "run this one run: instruction multiple times with different env vars" syntax?
There was a problem hiding this comment.
run is shell command, so we can likely have an array and iterate over it? (But I suppose it'd need a cd - command in addition, I suppose it's fine)
There was a problem hiding this comment.
Yeah, should have thought about that! :) A formatting error will be reported like that: https://github.com/rinja-rs/rinja/actions/runs/9905823875/job/27366138342?pr=69
There was a problem hiding this comment.
Sounds perfectly fine. 👍
Kijewski
force-pushed
the
pr-workspaces
branch
2 times, most recently
from
72c92f3
to
3043e0d
Compare
For one simple reason: I did not think about that obvious solution. :D In my brain it's hardwired "when iterating over subfolders in bash, then use |
Everytime you hit ctrl+s in VS code, the whole project in checked, which normally is a nice thing, and I would not like to disable that in my configuration. But in rinja, the diffent integration crates don't work well with one another and every single one will have compile errors in the automatic check, because `rinja_derive` won't know what the relevant `rinja_xyz` crate is. This renders the automatic check useless. Then it takes ages to compile and recompile all transitively used `proc_macros`. This PR moves all integration crates in there own, individual workspaces. `cargo check` will ignore them unless you traverse manually into their folders. The CI is set up to do just that.
| do | ||
| cd "$PKG" | ||
| echo "Testing: $PKG" | ||
| cargo fmt -- --check |
There was a problem hiding this comment.
Just one question: there is no more clippy check here. However I see there is one above. Is --all-targets run on all crates even if they are in different workspaces?
There was a problem hiding this comment.
Does --all-targets even do anything if you don't specify a target list somewhere? Isn't all=native in by default?
Clippy checking on +nightly is always stressful to me, because often tests are a bit over-eager in their early stage.
The clippy further up ensures that all packages are tested on +stable. If you run clippy in a workspace root, I think normally only default-members are tested, but I'm not sure.
There was a problem hiding this comment.
Hum... I don't mind running non-nightly clippy. Just want to be sure it'll check all crates, even those which are not in the default workspace.
There was a problem hiding this comment.
The list is exhaustive. Everything is checked. We just have to make sure, that the list stays exhaustive. I don't see use adding new crates every other day, so this requirement should be fine. :)
There was a problem hiding this comment.
Sorry, that's not what I meant. Let me try again. 😆
So now, the default workspace doesn't include all crates. So here, I was wondering if cargo clippy --all-targets was checking all crates, even if not in the default workspace. If not, could we make the same loop (and store the list of crate in a variable?) so we can run clippy (stable version) on all crates?
There was a problem hiding this comment.
I confused myself what "targets" are in this case. In other cases "target" is short for "target platform", but in here "target" is the same as in Make targets:
Build all targets. This is equivalent to specifying --lib --bins --tests --benches --examples.
No, clippy won't traverse into folders to find additional crates. If clippy is executed in a workspace root, the workspace is checked (I don't know if all members or only the default-members). If the current folder is not a workspace root, then the current package is tested.
In the current configuration not all packages are linted. In the new configuration all packages are linted. The new config is strictly better than the old one. :)
There was a problem hiding this comment.
So confusing. XD Thanks for the clarification! Then all good for me. :)
|
Thanks! |
Everytime you hit ctrl+s in VS code, the whole project is checked, which normally is a nice thing, and I would not like to disable that in my configuration.
But in rinja, the diffent integration crates don't work well with one another and every single one will have compile errors in the automatic check, because
rinja_derivewon't know what the relevantrinja_xyzcrate is. This renders the automatic check useless. Then it takes ages to compile and recompile all transitively usedproc_macros.This PR moves all integration crates in there own, individual workspaces.
cargo checkwill ignore them unless you traverse manually into their folders. The CI is set up to do just that.