Skip to content
This repository has been archived by the owner on Dec 9, 2024. It is now read-only.

Fix a few issues with the CI #361

Merged
merged 8 commits into from
Jan 10, 2024
Merged

Fix a few issues with the CI #361

merged 8 commits into from
Jan 10, 2024

Conversation

ariostas
Copy link
Member

@ariostas ariostas commented Jan 5, 2024

I fixed a few issues that came up after the CI was implemented. Here they are:

  • clang-tidy defaults to interpreting .h files as C code. The action used to run this check does not allow enough control over the command that is run to specify the right parameters. This issue is also not solved by a standard compile_commands.json compilation database since it typically only contains commands for .cc files. This was solved by using the compdb python package to also generate compilation commands for headers.
  • The version of clang-format and clang-tidy was 12, but now it was set to 16 to match the version in CMSSW 13_3_0.
  • The CI was implemented in a way that prevents equivalent jobs to run at the same time. However, there was a bug where new comments were cancelling previous jobs because the concurrency grouping was not precise enough.
  • When running the CMSSW checks, there is the option to specify a branch. There is a check of the branch name to prevent code injection, but the regex that was used was not generic enough. The check is now done with a git command, so it should be completely accurate.
  • It used to not be very visible when things were running in the CI. Now the CI adds associated checks to the commit that is being used.
  • There are now phony targets in the makefile to fix formatting and check for issues. Instructions were added to the readme.

There was also the issue of the linter not being able to post comments. I'm not sure why that is happening, but I found online that turning actions off and on again might solve this issue. I did this, so hopefully it will work now.

I will also update the CMSSW workflow so that it produces comparison plots instead of just validation plots. But that is all done in TrackLooper-actions so no changes need to be made here.

Makefile Outdated Show resolved Hide resolved
@ariostas
Copy link
Member Author

ariostas commented Jan 8, 2024

Following @slava77's suggestions I moved the format and check targets to the makefile in the SDL directory, which seems more appropriate. I also included more flags to clang-tidy to match the ones used during compilation. I had to make a modification to the suggested command to collect the include paths since including one of those paths caused a bunch of errors. There is an equivalent path, but with a "fixed" suffix, which works well.

I considered producing a compile_commands.json database and use the same python package that is used in the CI, but overall it makes things more complex and we get the same warnings/errors, so I didn't think it was worth it.

@ariostas
Copy link
Member Author

ariostas commented Jan 9, 2024

I made one last change. I'm currently testing producing comparison plots in the CMSSW workflow, but since it takes so long that the initial token that is generated in the job fails to be revoked at the end. So I modified it so that it no longer attempts to revoke it.

@slava77
Copy link
Contributor

slava77 commented Jan 9, 2024

I'm currently testing producing comparison plots in the CMSSW workflow

just to be sure, before pushing the "merge"; is that test passing now?

@ariostas
Copy link
Member Author

I just pushed the last changes to the TrackLooper-actions repo, so it's all ready now. Everything is working in the test repo, so it should work here too.

@slava77 slava77 merged commit d0effe6 into master Jan 10, 2024
1 check passed
@ariostas ariostas deleted the fix-ci branch January 30, 2024 20:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants