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

Replace count_loc.sh with xtask #328

Merged
merged 2 commits into from
Nov 20, 2024
Merged

Conversation

joshka
Copy link
Contributor

@joshka joshka commented Nov 13, 2024

To run the count_loc now, you can use cargo xtask count-loc instead of
./count_loc.sh.

See https://github.com/matklad/cargo-xtask for more info on
cargo-xtask.

Fixes: #323

@joshka
Copy link
Contributor Author

joshka commented Nov 13, 2024

image

Copy link

codecov bot commented Nov 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 48.41%. Comparing base (edcba19) to head (8890610).
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #328   +/-   ##
=======================================
  Coverage   48.41%   48.41%           
=======================================
  Files          10       10           
  Lines        1138     1138           
=======================================
  Hits          551      551           
  Misses        587      587           
Flag Coverage Δ
wasm32-wasip1 49.00% <ø> (ø)
x86_64-apple-darwin 49.09% <ø> (ø)
x86_64-pc-windows-gnu 48.29% <ø> (ø)
x86_64-unknown-linux-gnu 49.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

xtask/src/count_loc.rs Outdated Show resolved Hide resolved
Copy link
Owner

@ilai-deutel ilai-deutel left a comment

Choose a reason for hiding this comment

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

Amazing, thank you so much!

xtask/src/count_loc.rs Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
Cargo.toml Show resolved Hide resolved
@joshka joshka force-pushed the jm/xtask branch 5 times, most recently from 856e729 to d322c3c Compare November 15, 2024 00:29
@joshka
Copy link
Contributor Author

joshka commented Nov 15, 2024

Apologies for the rebases here - missed removing the intentional failure code and reverting it back properly (and then got caught by vscode autoformatting)

xtask/src/count_loc.rs Outdated Show resolved Hide resolved
xtask/src/count_loc.rs Show resolved Hide resolved
@joshka
Copy link
Contributor Author

joshka commented Nov 15, 2024

I'll take a look at the current set of failures a bit later

@joshka
Copy link
Contributor Author

joshka commented Nov 15, 2024

Rebased and squashed to re-trigger checks, I think the last failure might have been a transient rust nightly issue rather than a true failure. Re-running to confirm.

Edit: looks like not a transient problem.
https://github.com/ilai-deutel/kibi/actions/runs/11852342253/job/33030343047?pr=328

Run mbrobbel/rustfmt-check@master
  with:
    token: ***
    mode: review
    commit-message: Format Rust code using rustfmt
  env:
    CARGO_TERM_COLOR: always
/home/runner/.cargo/bin/cargo +nightly fmt -- --emit json
[]
[]
Error: Unexpected non-whitespace character after JSON at position 2

Locally (latest nightly, on macOS) that command only shows a single [].
Edit: actually it's producing the same output now. I was mistaken

@joshka
Copy link
Contributor Author

joshka commented Nov 15, 2024

with --package: a different error appears...

Run mbrobbel/rustfmt-check@master
  with:
    token: ***
    mode: review
    args: --package kibi
    commit-message: Format Rust code using rustfmt
  env:
    CARGO_TERM_COLOR: always
/home/runner/.cargo/bin/cargo +nightly fmt --package kibi -- --emit json
[]
Error: Resource not accessible by integration

@joshka
Copy link
Contributor Author

joshka commented Nov 15, 2024

Raised mbrobbel/rustfmt-check#1137 against the rustfmt check action to try to find out more about the problem.

@joshka
Copy link
Contributor Author

joshka commented Nov 15, 2024

mbrobbel/rustfmt-check#1137 (comment)

Looks like this is happening on a PR from a forked repo. Depending on the repo configuration the token may not get the required write permissions, resulting in the Resource not accessible by integration error. More info here https://docs.github.com/en/actions/security-for-github-actions/security-guides/automatic-token-authentication#modifying-the-permissions-for-the-github_token.

Can you maybe rerun the failed job and select Enable debug logging.?

@ilai-deutel
Copy link
Owner

Thank you for raising this with the action owner! I added this yesterday to reduce the friction with fmt checks and it worked when I tested it in a different PR, I'm not sure what's going on. I shared the debug logs in the other issue.

@ilai-deutel
Copy link
Owner

I just saw you modified the CI YAML file. I wonder if this is a security feature, disabling token permissions for non-owners who modify the config files to avoid running arbitrary code with all the permissions.

Could you try maybe adding rustfmt::skip codeblocks in the xtask code as needed, and revert your change to the yaml?

@ilai-deutel
Copy link
Owner

I disabled mbrobbel/rustfmt-check in the workflow config for now

To run the count_loc now, you can use `cargo xtask count-loc` instead of
`./count_loc.sh`.

See <https://github.com/matklad/cargo-xtask> for more info on
cargo-xtask.

Fixes: ilai-deutel#323
There's a +nightly oddity where it returns an empty array twice instead of valid json for multiple projects, this causes the check action to fail
@joshka
Copy link
Contributor Author

joshka commented Nov 20, 2024

Rebased on master - all green

@ilai-deutel ilai-deutel merged commit d9db901 into ilai-deutel:master Nov 20, 2024
26 checks passed
@joshka joshka deleted the jm/xtask branch November 20, 2024 03:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace count_loc with a Rust script to ensure it is cross-platform
2 participants