IWYU failing on develop and also fails too early

[TGWDB]

As seen on a recent PR, the IWYU CI job is currently failing on develop (likely due to parallel changes), example output here: https://github.com/diffblue/cbmc/actions/runs/3540003949/jobs/5945959712

This should be fixed (but not in the release PR since I want to keep that clean).

Further, this highlights a potential issue in the IWYU CI job that it fails immediately:

if sed '/minisat2-src/,/^--$/d' includes.txt | grep '^- ' -B1 ; then
    echo "Unnecessary includes found. Use '// IWYU pragma: keep' to override this."
    exit 1
  fi

It might be a good idea to output all failures and then fail?

[tautschnig]

I am not sure why this is deemed to be failing early?

[TGWDB]

I am not sure why this is deemed to be failing early?

Sorry if this is not clear. The script exits immediately if any IWYU compliance violation is found:

if sed '/minisat2-src/,/^--$/d' includes.txt | grep '^- ' -B1 ; then
  echo "Unnecessary includes found. Use '// IWYU pragma: keep' to override this."
  exit 1
fi

I would propose to rewrite something like (pseudocode):

NONCOMPLIANT=false
if sed '/minisat2-src/,/^--$/d' includes.txt | grep '^- ' -B1 ; then
  echo "Unnecessary includes found. Use '// IWYU pragma: keep' to override this."
  NONCOMPLIANT=true
fi
if NONCOMPLIANT; then
  exit 1
fi

This allows all the non-compliant instances to be shown and then all resolved in one pass.

[tautschnig]

Hmm, the only concern I really share is the use of -B1: this isn't a for loop that exits early, it will actually grep all the output produced by include-what-you-use, which has run to completion beforehand. And even with the -B1: the full output (i.e., the contents of what ends up as includes.txt) is dumped to the log file.

[TGWDB]

Hmm, the only concern I really share is the use of -B1: this isn't a for loop that exits early, it will actually grep all the output produced by include-what-you-use, which has run to completion beforehand. And even with the -B1: the full output (i.e., the contents of what ends up as includes.txt) is dumped to the log file.

Yes. I'm not sure the best way to resolve here, but exiting without all the annotations is annoying for someone who has to keep rerunning to get them. On the other hand, the output is pretty verbose, so showing only the important ones would be good...

[tautschnig]

Yes. I'm not sure the best way to resolve here, but exiting without all the annotations is annoying for someone who has to keep rerunning to get them. On the other hand, the output is pretty verbose, so showing only the important ones would be good...

I might have misunderstood, but is the only remaining concern that the final summary uses -B1 and, thereby, may fail to list some of the unnecessary includes in the summary? The output prior to that has all unnecessary includes as well as include-what-you-use's recommendation on what to use instead. If I do understand correctly, then the fix is to use sed or perl instead of grep -B1 to have all the lines. This was something that bit me when doing the one-off cleanup in #7235, but this CI job should make it unlikely that anyone ever has to do repeat cleanup again. Or people could just run the one-liner that the CI job runs locally.

[TGWDB]

Yes. I'm not sure the best way to resolve here, but exiting without all the annotations is annoying for someone who has to keep rerunning to get them. On the other hand, the output is pretty verbose, so showing only the important ones would be good...

I might have misunderstood, but is the only remaining concern that the final summary uses -B1 and, thereby, may fail to list some of the unnecessary includes in the summary? The output prior to that has all unnecessary includes as well as include-what-you-use's recommendation on what to use instead. If I do understand correctly, then the fix is to use sed or perl instead of grep -B1 to have all the lines. This was something that bit me when doing the one-off cleanup in #7235, but this CI job should make it unlikely that anyone ever has to do repeat cleanup again. Or people could just run the one-liner that the CI job runs locally.

Yes. This bit me because I was going to wrap the fix up myself but I couldn't run locally (iwyu issue), so seeing everything in the CI run would have been good.

[tautschnig]

Yes. This bit me because I was going to wrap the fix up myself but I couldn't run locally (iwyu issue), so seeing everything in the CI run would have been good.

Well, as said it's all there, you would just need to browse further up.

[TGWDB]

Well, as said it's all there, you would just need to browse further up.

Yes, it was there in 50,000 lines of logs. Now having spent more time with it I can better understand what I'm looking for, but yesterday with limited time and an unexpected failure that I could not reproduce locally it was annoying to try to sort out.

I agree that if it all passes now then it should be ok though.