converted actionlint github workflow test to rust test #560

markv44 · 2025-05-02T19:10:47Z

This pr partially fixes #413
Converted actionlint workflow test to rust test

markv44 · 2025-05-02T19:44:17Z

Hi @smoelius, I tried the actionlint conversion from #413 . Please review and let me know if you have any questions.

smoelius · 2025-05-03T18:26:23Z

test-fuzz/tests/integration/ci.rs

+        ])
+        .logged_assert()
+        .success();
+    let home = home::home_dir().unwrap();


std::env::home_dir will be undeprecated in Rust 1.87, which will be released in a couple of weeks: rust-lang/rust#140133

Could we please use that with #[allow(deprecated)]?

Also, I would prefer to continue to install actionlint from ci.yml. Could you please move the command to do that to just after this line?

test-fuzz/.github/workflows/ci.yml

Line 169 in d448ab4

cargo install group-runner || true

It looks like actionlint is warning following your changes. I will look into the warnings separately.

Thank you @smoelius for the feedback — I just need a bit of clarification.

I would prefer to continue to install actionlint from ci.yml. Could you please move the command to do that to just after this line?

The Rust actionlint test I wrote currently does two things:

Installs the actionlint tool

Runs the installed actionlint binary to confirm it works

From your comment, I understand you'd prefer to keep the installation in ci.yml. Does that mean I should remove the installation step from the Rust function and leave only the part that runs the binary?

Also, since the ci.yml file still contains the actionlint test (I haven’t removed it yet), if we're keeping the installation there, should I move that step down to line 170 as you suggested?

Apologies if these are overly basic questions — I just want to make sure I’m fully aligned before making changes. Thanks again for your helpful feedback!

Apologies if these are overly basic questions

Not basic questions. Sorry for being unclear.

From your comment, I understand you'd prefer to keep the installation in ci.yml. Does that mean I should remove the installation step from the Rust function and leave only the part that runs the binary?

Yes, please.

Also, since the ci.yml file still contains the actionlint test (I haven’t removed it yet), if we're keeping the installation there, should I move that step down to line 170 as you suggested?

Yes, please.

I would prefer that the test not install a binary into the user's home directory. So, for now, I think it is fine to require the user to already have actionlint installed.

Moving the installation is required because actionlint will now be run from the test job (via ci.rs) rather than the lint job.

markv44 · 2025-05-05T12:37:29Z

Thank you @smoelius for the response. I have applied the changes you requested.

Looks like the CI is still failing. I have checked the logs, and it looks like the CI is suggesting these changes

../.github/workflows/ci.yml:30:9: shellcheck reported issue in this script: SC2129:style:1:1: Consider using { cmd1; cmd2; } >> file instead of individual redirects [shellcheck]

for this one, I was thinking about grouping echo commands in GitHub Actions summary step. Dylint codebase action summary step currently has the grouped echo commands.

Current:

- name: Log github refs
  run: |
    echo '```' >> "$GITHUB_STEP_SUMMARY"
    echo 'github.ref: ${{ github.ref }}' >> "$GITHUB_STEP_SUMMARY"
    echo 'github.sha: ${{ github.sha }}' >> "$GITHUB_STEP_SUMMARY"
    echo '```' >> "$GITHUB_STEP_SUMMARY"

Proposed:

- name: Log github refs
  run: |
    {
      echo '```'
      echo 'github.ref: ${{ github.ref }}'
      echo 'github.sha: ${{ github.sha }}'
      echo '```'
    } >> "$GITHUB_STEP_SUMMARY"

../.github/workflows/release.yml:26:9: shellcheck reported issue in this script: SC2002:style:7:9: Useless cat. Consider 'cmd < file | ..' or 'cmd file | ..' instead [shellcheck]

and for this one, simplifying cat | tee usage in release.yml

Current:

cat "$TMP" |
tee /dev/stderr |
tail -n 1 |
grep '^.*: crate [^`]* already exists on crates.io index$'

Proposed:

tee /dev/stderr < "$TMP" |
tail -n 1 |
grep '^.*: crate [^`]* already exists on crates.io index$'

These changes will have the following improvements:

Readability by grouping related commands
Reduces duplication of the file redirection
Shell idiomaticity by avoiding unnecessary cat
Slightly reduces process overhead
Cleaner and more concise syntax

Can you please check if the changes I mentioned are valid or not?
If it is, then can I create a separate PR for these fixes?
I have test these changes locally, and the it worked. (the build did not fail)

Thank You!

smoelius · 2025-05-06T00:14:15Z

.github/workflows/ci.yml


+      - name: Install actionlint
+        run: go install github.com/rhysd/actionlint/cmd/actionlint@latest


Suggested change

- name: Install actionlint

run: go install github.com/rhysd/actionlint/cmd/actionlint@latest

go install github.com/rhysd/actionlint/cmd/actionlint@latest

Sorry, I was unclear. You can put the go install ... after the last cargo install ... in the "Install tools" step.

Please also don't forget to use std::env::home_dir instead of home::home_dir (and remove home).

smoelius · 2025-05-06T00:21:08Z

@markv44 Thanks very much for your analysis of the actionlint warnings. I will respond within a day or so.

smoelius · 2025-05-06T14:51:16Z

I just pushed a change that made the "Log github refs" code go away.

For this one:

Current:

cat "$TMP" |
tee /dev/stderr |
tail -n 1 |
grep '^.*: crate [^`]* already exists on crates.io index$'

Proposed:

tee /dev/stderr < "$TMP" |
tail -n 1 |
grep '^.*: crate [^`]* already exists on crates.io index$'

I actually prefer the cat ... | style. I think it makes more clear where tee's input is coming from.

I've not tried this before, but could you please try running actionlint with SHELLCHECK_OPTS="--exclude=SC2002" (based on this) and see if that makes the error go away?

markv44 · 2025-05-07T06:45:22Z

@smoelius I've added SHELLCHECK_OPTS="--exclude=SC2002" as you suggested. I hope I've implemented it correctly.

In the CI failure logs, the error about grouping echo commands is gone, but the warning about 'useless cat' in the release.yml file is still appearing.

smoelius

I think we're just about there. But let's please check that CI passes after making these changes.

test-fuzz/tests/integration/ci.rs

smoelius · 2025-05-07T10:18:26Z

test-fuzz/tests/integration/ci.rs

@@ -232,7 +232,16 @@ fn unmaintained() {
        .success();
 }

+#[allow(deprecated)]


Suggested change

#[allow(deprecated)]

test-fuzz/tests/integration/ci.rs

markv44 · 2025-05-07T11:43:23Z

Hi @smoelius, thank you for your feedback. I've applied the changes and the useless cat error is also gone now.

But there's one new error about cargo-udeps

code=1
stdout=```
unused dependencies:
test-fuzz v7.2.0 (/home/runner/work/test-fuzz/test-fuzz/test-fuzz)
└─── dev-dependencies
└─── "home"
Note: They might be false-positive.
For example, `cargo-udeps` cannot detect usage of crates that are only used in doc-tests.
To ignore some dependencies, write `package.metadata.cargo-udeps.ignore` in Cargo.toml.

I am not sure what this error is about. Can you give some suggestions?

Thank you!

smoelius · 2025-05-07T11:51:08Z

The error is saying that the home dependency is unneeded and can be removed.

Recall that I requested you use std::env::home_dir instead of home::home_dir.

markv44 · 2025-05-07T13:11:52Z

@smoelius thank you for the help. The CI tests passed

smoelius · 2025-05-07T13:35:03Z

Cargo.toml

@@ -34,6 +34,7 @@ mio = { version = "1.0", features = ["os-ext", "os-poll"] }
 num_cpus = "1.16"
 num-traits = "0.2"
 option_set = "0.3"
+home = "0.5"


Suggested change

home = "0.5"

Oof. It's unfortunate that cargo-udeps doesn't flag workspace dependencies. Could you please remove this too?

After that, could you please squash your commits down to one?

markv44 · 2025-05-07T15:31:58Z

@smoelius I think everything is done!

smoelius · 2025-05-07T15:45:21Z

Sorry, I should have said "rebase onto master and squash your commits down to one". Right now, it looks like 569ac11 includes changes that are already in master.

Could you please try this?

git checkout actionlint-ci-test
git pull https://github.com/trailofbits/test-fuzz master --rebase
git push --force

Ultimately, I am going to rebase your changes when I merge, but it is nice to seem them beforehand for sanity.

markv44 · 2025-05-07T15:57:43Z

Thanks for clarification @smoelius. I believe it is done correctly this time.

smoelius · 2025-05-07T16:56:05Z

Thanks, @markv44!

If you wanted to submit a follow-up PR to remove the Actionlint step from ci.yml, I would appreciate it. No obligation, though.

markv44 requested a review from smoelius as a code owner May 2, 2025 19:10

smoelius reviewed May 3, 2025

View reviewed changes

smoelius reviewed May 6, 2025

View reviewed changes

smoelius reviewed May 7, 2025

View reviewed changes

markv44 force-pushed the actionlint-ci-test branch 2 times, most recently from 8e23e85 to 569ac11 Compare May 7, 2025 14:29

converted actionlint github workflow test to rust test

3ba6e77

markv44 force-pushed the actionlint-ci-test branch from 5bb0a64 to 3ba6e77 Compare May 7, 2025 15:56

smoelius added this pull request to the merge queue May 7, 2025

Merged via the queue into trailofbits:master with commit f31d1e3 May 7, 2025
20 checks passed


		- name: Install actionlint
		run: go install github.com/rhysd/actionlint/cmd/actionlint@latest

converted actionlint github workflow test to rust test #560

converted actionlint github workflow test to rust test #560

Uh oh!

Conversation

markv44 commented May 2, 2025

Uh oh!

markv44 commented May 2, 2025

Uh oh!

smoelius May 3, 2025

Choose a reason for hiding this comment

Uh oh!

markv44 May 4, 2025

Choose a reason for hiding this comment

Uh oh!

smoelius May 5, 2025

Choose a reason for hiding this comment

Uh oh!

markv44 commented May 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smoelius May 6, 2025

Choose a reason for hiding this comment

Uh oh!

smoelius commented May 6, 2025

Uh oh!

smoelius commented May 6, 2025

Uh oh!

markv44 commented May 7, 2025

Uh oh!

smoelius left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

smoelius May 7, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

markv44 commented May 7, 2025

Uh oh!

smoelius commented May 7, 2025

Uh oh!

markv44 commented May 7, 2025

Uh oh!

smoelius May 7, 2025

Choose a reason for hiding this comment

Uh oh!

markv44 commented May 7, 2025

Uh oh!

smoelius commented May 7, 2025

Uh oh!

markv44 commented May 7, 2025

Uh oh!

smoelius commented May 7, 2025

Uh oh!

Uh oh!

Uh oh!

markv44 commented May 5, 2025 •

edited

Loading