Skip to content

Use Ubuntu-provided git-clang-format in CI check #7160

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 28, 2022
Merged

Use Ubuntu-provided git-clang-format in CI check #7160

merged 1 commit into from
Sep 28, 2022

Conversation

tautschnig
Copy link
Collaborator

The 14.0.6 version of git-clang-format (which was introduced in b377779) appears to be incompatible with clang-format-11. The result was that context lines of the diff got formatted as well, as seen in
https://github.com/diffblue/cbmc/actions/runs/3141827465/jobs/5104706348.

Using the distribution-provided version of git-clang-format fixes this problem.

  • Each commit message has a non-empty body, explaining why the change was made.
  • n/a Methods or procedures I have added are documented, following the guidelines provided in CODING_STANDARD.md.
  • n/a The feature or user visible behaviour I have added or modified has been documented in the User Guide in doc/cprover-manual/
  • n/a Regression or unit tests are included, or existing tests cover the modified code (in this case I have detailed which ones those are in the commit message).
  • n/a My commit message includes data points confirming performance improvements (if claimed).
  • My PR is restricted to a single feature or bugfix.
  • n/a White-space or formatting changes outside the feature-related changed lines are in commits of their own.

@tautschnig tautschnig requested a review from a team as a code owner September 28, 2022 08:26
@tautschnig tautschnig mentioned this pull request Sep 28, 2022
Merged
4 tasks
@codecov
Copy link

codecov bot commented Sep 28, 2022 •
edited
Loading

Codecov Report

Base: 77.88% // Head: 77.88% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (6d6319f) compared to base (c764c1d).
Patch has no changes to coverable lines.

Additional details and impacted files 
@@           Coverage Diff            @@
##           develop    #7160   +/-   ##
========================================
  Coverage    77.88%   77.88%           
========================================
  Files         1576     1576           
  Lines       181856   181859    +3     
========================================
+ Hits        141645   141648    +3     
  Misses       40211    40211
Impacted Files Coverage Δ
src/util/expr.h 97.02% <ø> (ø)
src/ansi-c/ansi_c_entry_point.cpp 89.57% <0.00%> (+0.12%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

NlightNFotis
NlightNFotis reviewed Sep 28, 2022
View reviewed changes
.github/workflows/pull-request-checks.yaml Outdated
wget -O git-clang-format https://raw.githubusercontent.com/llvm/llvm-project/llvmorg-14.0.6/clang/tools/clang-format/git-clang-format
chmod u+x git-clang-format
mv git-clang-format /usr/local/bin
sudo apt-get install --no-install-recommends -yq clang-format clang-format-11
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this installing both clang-format and clang-format-11?

Mind you that both appear to install a git-clang-format binary, the first one installs git-clang-format-10 and the second git-clang-format-11. Surely these clash in terms of path?

https://packages.ubuntu.com/focal-updates/amd64/clang-format-11/filelist

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The clang-format package installs git-clang-format, while the others install one with a version suffix. We need/want the one without a version suffix, hence the need to install both clang-format as well as clang-format-11.

Copy link
Contributor

Choose a reason for hiding this comment

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

So the no suffix version of git-clang-format wouldn't be needed, if we just update .github/workflows/pull-request-check-clang-format.sh to run git-clang-format-11 instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think that's true, yes. I'll give that one a try.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

@tautschnig
Use Ubuntu-provided git-clang-format in CI check
6d6319f 
The 14.0.6 version of git-clang-format (which was introduced in
b377779) appears to be incompatible with clang-format-11. The
result was that context lines of the diff got formatted as well, as seen
in
https://github.com/diffblue/cbmc/actions/runs/3141827465/jobs/5104706348.

Using the distribution-provided version of git-clang-format fixes this
problem.
@tautschnig tautschnig force-pushed the bugfixes/git-clang-format branch from bb13899 to 6d6319f Compare September 28, 2022 13:39
thomasspriggs
thomasspriggs approved these changes Sep 28, 2022
View reviewed changes
Copy link
Contributor

@thomasspriggs thomasspriggs left a comment

Choose a reason for hiding this comment

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

👍

@tautschnig tautschnig merged commit 5ebbdcf into diffblue:develop Sep 28, 2022
@tautschnig tautschnig deleted the bugfixes/git-clang-format branch September 28, 2022 14:51
@tautschnig tautschnig restored the bugfixes/git-clang-format branch September 29, 2022 10:34
@tautschnig tautschnig deleted the bugfixes/git-clang-format branch September 29, 2022 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NlightNFotis NlightNFotis NlightNFotis approved these changes

@thomasspriggs thomasspriggs thomasspriggs approved these changes

Assignees

@thomasspriggs thomasspriggs

Labels
bugfix CI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tautschnig @NlightNFotis @thomasspriggs