build: move from `make` to `cargo xtask` workflows #2297

ErichDonGubler · 2023-03-31T17:19:01Z

~~Is this too slow for CI?~~ Building xtask fully every time is relatively expensive (1m30-2m00 from my most recent tests), but then the only real resolution is caching. Current stats:
- ~360 MB (~90 MB avg. caching per job at 4 jobs) of cache storage that will only get invalidated on xtask dep. changes
- ~45sec additional latencies for cache-warm recompilation of xtask on Windows jobs
- 10-15sec additional latency for cache-warm recompilation of xtask on Linux (1) and Mac (2) jobs

Re-implement naga development workflows using cargo xtask. Convert make logic and shader test configuration as file with Bash variables into an xtask crate and YAML files, respectively.

Pros:

We now have a portable workflow everywhere, which means Windows folks and people who don't install make don't have to suffer. 😮‍💨
Workflow logic is now relatively easy to inspect and change. Whew! 💁🏻‍♂️💦
Contributors can use their existing Rust knowledge to contribute to developer experience. 🎉
cargo xtask is a relatively well-known convention for workflows in the ecosystem.
We can do fancy things like allow folks to run at different log levels for workflows, depending on their tastes.

Cons:

There's now a non-trivial compile step to project workflow. Incremental rebuilds seem to be pretty short, though!
Code is much more verbose than the (very) terse make implementation.

tests/out/hlsl/array-in-ctor.yaml

xtask/.gitignore

codecov-commenter · 2023-04-03T17:04:27Z

Codecov Report

Merging #2297 (205ee31) into master (53d62b9) will decrease coverage by 0.07%.
The diff coverage is 30.18%.

❗ Current head 205ee31 differs from pull request most recent head 87afaf8. Consider uploading reports for the commit 87afaf8 to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##           master    #2297      +/-   ##
==========================================
- Coverage   82.19%   82.13%   -0.07%     
==========================================
  Files          82       83       +1     
  Lines       44267    44324      +57     
==========================================
+ Hits        36384    36404      +20     
- Misses       7883     7920      +37

Impacted Files	Coverage Δ
xtask/hlsl-snapshots/src/lib.rs	`30.18% <30.18%> (ø)`

... and 3 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

ErichDonGubler · 2023-04-03T19:51:24Z

xtask/src/main.rs

+                    &mut found_err,
+                    |file, _| {
+                        // TODO: strip prefix to match old Makefile output?
+                        log::info!("Validating {}", file.display());


note: N.B. that this output is technically different from the original Makefile; it includes the full relative path parent prefix of the glob search path, instead of "just" printing the base name. I don't think anybody will care, tho, because it makes this particular branch consistent with everything else!

Makefile

tests/out/hlsl/access.ron

ErichDonGubler · 2023-04-11T14:02:45Z

This PR should basically be ready for full review and merging. 😤😀

ErichDonGubler · 2023-06-06T18:45:43Z

Ping for feedback on this (CC @teoxoy, @cwfitzgerald, @jimblandy); I keep rebasing, waiting for some feedback, but nobody's requesting changes or asking questions. AFAIK, there are no open questions or concerns. Merge plz?

teoxoy

Looks solid! Much easier to follow than the Makefile.

xtask/src/main.rs

xtask/hlsl-snapshots/src/lib.rs

xtask/src/main.rs

xtask/src/glob.rs

Originally implemented in [`naga`gfx-rs#2297](gfx-rs#2297), but removed as an experiment to see who actually needs the workflow. Reverts commit 337c760.

ErichDonGubler · 2023-06-07T17:36:32Z

Resolved some clippy lints with 5a659c8. Hmm…maybe we should be running clippy on xtask too? 🤔

ErichDonGubler · 2023-06-07T18:20:54Z

@teoxoy: Once you've reviewed, or you have more feedback, most of the fixup!s here should squash down neatly with git rebase -i --autosquash. I've already done it locally, and can push when you're ready.

teoxoy

LGTM

teoxoy

Ah, one last thing: Could you please add the path of the new xtask crate to the validation workflows so that the CI gets triggered if we modify it in the future? Thanks!

ErichDonGubler · 2023-06-08T14:44:58Z

@teoxoy: Done with 4858ac2, already auto-squashed.

Re-implement `naga` development workflows using [`cargo xtask`]. Convert `make` logic and shader test configuration as file with Bash variables into an `xtask` crate and YAML files, respectively. Pros: * We now have a _portable_ workflow everywhere, which means Windows folks and people who don't install `make` don't have to suffer. 😮‍💨 * Workflow logic is now relatively easy to inspect and change. Whew! 💁🏻‍♂️💦 * Contributors can use their existing Rust knowledge to contribute to developer experience. 🎉 * `cargo xtask` is a relatively well-known convention for workflows in the ecosystem. * We can do fancy things like allow folks to run at different log levels for workflows, depending on their tastes. Cons: * There's now a non-trivial compile step to project workflow. Incremental rebuilds seem to be pretty short, though! * Code is much more verbose than the (very) terse `make` implementation. [`cargo xtask`]: https://github.com/matklad/cargo-xtask

Originally implemented in [`naga`gfx-rs#2297](gfx-rs#2297), but removed as an experiment to see who actually needs the workflow. Reverts commit 337c760.

ErichDonGubler commented Mar 31, 2023

View reviewed changes

tests/out/hlsl/array-in-ctor.yaml Outdated Show resolved Hide resolved

ErichDonGubler commented Mar 31, 2023

View reviewed changes

xtask/.gitignore Outdated Show resolved Hide resolved

This comment was marked as outdated.

Sign in to view

ErichDonGubler force-pushed the xtask branch 5 times, most recently from 883c96f to 114d6d6 Compare April 3, 2023 16:43

ErichDonGubler force-pushed the xtask branch 4 times, most recently from 8916401 to 1108f3d Compare April 3, 2023 18:34

ErichDonGubler commented Apr 3, 2023

View reviewed changes

ErichDonGubler force-pushed the xtask branch 2 times, most recently from 205ee31 to 87afaf8 Compare April 4, 2023 15:53

ErichDonGubler commented Apr 4, 2023

View reviewed changes

Makefile Show resolved Hide resolved

ErichDonGubler commented Apr 4, 2023

View reviewed changes

tests/out/hlsl/access.ron Outdated Show resolved Hide resolved

ErichDonGubler force-pushed the xtask branch from 87afaf8 to 9e88bb6 Compare April 4, 2023 20:19

This comment was marked as resolved.

Sign in to view

ErichDonGubler marked this pull request as ready for review April 4, 2023 20:25

ErichDonGubler force-pushed the xtask branch 2 times, most recently from 9e17f9d to 68170b4 Compare April 10, 2023 14:27

This comment was marked as resolved.

Sign in to view

ErichDonGubler force-pushed the xtask branch from 68170b4 to 720bfcb Compare April 14, 2023 14:32

ErichDonGubler force-pushed the xtask branch 2 times, most recently from 44ce4f3 to 7a73050 Compare May 22, 2023 20:43

ErichDonGubler force-pushed the xtask branch 2 times, most recently from ece329e to 5b15581 Compare June 6, 2023 18:41

ErichDonGubler force-pushed the xtask branch from 5b15581 to 1491e55 Compare June 6, 2023 18:44

ErichDonGubler self-assigned this Jun 6, 2023

ErichDonGubler requested a review from cwfitzgerald June 6, 2023 18:45

teoxoy requested changes Jun 7, 2023

View reviewed changes

This comment was marked as resolved.

Sign in to view

ErichDonGubler force-pushed the xtask branch from 337c760 to 224da53 Compare June 7, 2023 18:08

This comment was marked as resolved.

Sign in to view

ErichDonGubler force-pushed the xtask branch from 224da53 to 39b6dca Compare June 7, 2023 18:13

teoxoy approved these changes Jun 8, 2023

View reviewed changes

teoxoy requested changes Jun 8, 2023

View reviewed changes

ErichDonGubler force-pushed the xtask branch from 39b6dca to bb288e1 Compare June 8, 2023 14:38

ErichDonGubler force-pushed the xtask branch from 8112408 to 4858ac2 Compare June 8, 2023 14:45

ErichDonGubler added 3 commits June 8, 2023 10:46

build(ci): trigger for ron files in HLSL shader snapshot test config.

9a83218

style: alphabetize dev-dependencies

8982be4

ErichDonGubler force-pushed the xtask branch from 4858ac2 to 8982be4 Compare June 8, 2023 14:47

ErichDonGubler requested a review from teoxoy June 8, 2023 14:48

teoxoy approved these changes Jun 8, 2023

View reviewed changes

teoxoy merged commit 273ff5d into gfx-rs:master Jun 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: move from `make` to `cargo xtask` workflows #2297

build: move from `make` to `cargo xtask` workflows #2297

ErichDonGubler commented Mar 31, 2023 •

edited

Loading

This comment was marked as outdated.

codecov-commenter commented Apr 3, 2023 •

edited

Loading

ErichDonGubler Apr 3, 2023 •

edited

Loading

This comment was marked as resolved.

This comment was marked as resolved.

ErichDonGubler commented Apr 11, 2023

ErichDonGubler commented Jun 6, 2023 •

edited

Loading

teoxoy left a comment

ErichDonGubler commented Jun 7, 2023 •

edited

Loading

This comment was marked as resolved.

This comment was marked as resolved.

ErichDonGubler commented Jun 7, 2023

teoxoy left a comment

teoxoy left a comment

ErichDonGubler commented Jun 8, 2023 •

edited

Loading

build: move from make to cargo xtask workflows #2297

build: move from make to cargo xtask workflows #2297

Conversation

ErichDonGubler commented Mar 31, 2023 • edited Loading

This comment was marked as outdated.

codecov-commenter commented Apr 3, 2023 • edited Loading

Codecov Report

ErichDonGubler Apr 3, 2023 • edited Loading

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

ErichDonGubler commented Apr 11, 2023

ErichDonGubler commented Jun 6, 2023 • edited Loading

teoxoy left a comment

Choose a reason for hiding this comment

ErichDonGubler commented Jun 7, 2023 • edited Loading

This comment was marked as resolved.

This comment was marked as resolved.

ErichDonGubler commented Jun 7, 2023

teoxoy left a comment

Choose a reason for hiding this comment

teoxoy left a comment

Choose a reason for hiding this comment

ErichDonGubler commented Jun 8, 2023 • edited Loading

build: move from `make` to `cargo xtask` workflows #2297

build: move from `make` to `cargo xtask` workflows #2297

ErichDonGubler commented Mar 31, 2023 •

edited

Loading

codecov-commenter commented Apr 3, 2023 •

edited

Loading

ErichDonGubler Apr 3, 2023 •

edited

Loading

ErichDonGubler commented Jun 6, 2023 •

edited

Loading

ErichDonGubler commented Jun 7, 2023 •

edited

Loading

ErichDonGubler commented Jun 8, 2023 •

edited

Loading