Ci rustfmt v2

[catenacyber]

Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/3836

Describe changes:

• GitHub action to check that rustfmt-ed files are still rustfmt-ed
• rustfmt some more files which have a small diff

#9044 with review taken into account

@jasonish would you know if there is some Github CI way to have both master and PR branch tested : so that we can check rustfmt status of files between without keeping a list of known rustfmt-ed files ?

Good start. While I'd rather just format master and master-6.0.x in one go at the same time, this is probably more realistic approach to get to whats expected of Rust projects today.

I am happy with the one-go approach but I guess the incremental one has more chances of getting merged...

[codecov]

Codecov Report

Merging #9072 (2de5151) into master (643e674) will decrease coverage by 0.07%.
The diff coverage is 66.66%.

❗ Current head 2de5151 differs from pull request most recent head d19275a. Consider uploading reports for the commit d19275a to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #9072      +/-   ##
==========================================
- Coverage   82.35%   82.28%   -0.07%     
==========================================
  Files         969      969              
  Lines      273655   273654       -1     
==========================================
- Hits       225359   225189     -170     
- Misses      48296    48465     +169     

Flag	Coverage Δ
fuzzcorpus	64.54% <33.33%> (-0.04%)	⬇️
suricata-verify	60.63% <0.00%> (-0.02%)	⬇️
unittests	62.91% <33.33%> (+<0.01%)	⬆️

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

[suricata-qa]

Information: QA ran without warnings.

Pipeline 14828

[suricata-qa]

Information:

field	baseline	test	%
	TREX_GENERIC_stats_chk
.capture.kernel_drops	0	461	0.00

Pipeline 14830

[jasonish]

@jasonish would you know if there is some Github CI way to have both master and PR branch tested : so that we can check rustfmt status of files between without keeping a list of known rustfmt-ed files ?

The commits.yml workflow does something like that.. But it'll just be easier if we can do it all at once.

[roligugus]

This will print diffs of all files, not just the current file.

Probably meant git diff -- $i?

[catenacyber]

But cargo fmt $i only changed one file, right ?

[roligugus]

Hmm, looking at that at https://github.com/rust-lang/rustfmt#running

Seems cargo fmt does the whole crate/module. It might silently ignore the file?

You'd have to use rustfmt it seems with files. However, rustfmt might also check some included files if I got the description correct?! So that might check transiently included files multiple times? Ugh, that would open up "interesting behaviours"... Maybe worth trying it out?

If that is the case, it might be easier to run cargo fmt once and then git diff check for the list of files in rustfmt.txt?

[catenacyber]

Thanks Roland for the tips, waiting for a decision wether this is the right approach before tuning this

[roligugus]

Nothing wrong with this check, just FYI.

An alternative to check for differences would be git diff --quiet -- $i which makes git diff "exit with codes similar to diff". I.e. returns 1 if there are any differences, 0 otherwise. See git help diff

[catenacyber]

Thanks, looks nicer.

What is the exact bash syntax for the test ?

[roligugus]

Different ways to skin that cat.

You might have seen something like:

git diff --quiet -- $i
if [ $? -ne 0 ] ; then
   # multiple commands ...
fi

I'd probably would do (if you don't need the exit value of the command):

if ! git diff --quiet -- $i; then
   # multiple commands ...
fi

Note the inversion with ! ... as the then path is executed if the command returns 0. diff/git diff --quiet returns 1 (not 0 basically) if files are different.

Or you could short-circuit things

git diff --quiet -- $i || ( 
   # multiple commands ...
)

Or if you fancy one liners - not my personal preference if more than one command in failure path, but valid as well:

git diff --quiet -- $i || ( git diff -- $i; echo "$i needs formatting"; r=1 )

It's really your personal preference

[catenacyber]

Should I close this, tracked by https://redmine.openinfosecfoundation.org/issues/3836 because the plan is to rustfmt all code when starting on 8 branch ?

[victorjulien]

Should I close this, tracked by https://redmine.openinfosecfoundation.org/issues/3836 because the plan is to rustfmt all code when starting on 8 branch ?

Probably best, ya.