Skip to content

Conversation

@wwieclaw
Copy link
Contributor

@wwieclaw wwieclaw commented Jan 20, 2023

Description

@swift-nav/devinfra

Adding a Makefile into the c directory for commonly used bazel commands to make it easier for developers to start using bazel.

Added targets:

  • do-all-unit-tests
  • do-all-integration-tests
  • clang-format-all-check
  • clang-format-all
  • clang-tidy-all-check
  • do-code-coverage
  • do-generate-coverage-report

I'm using aspects to run clang-format and clang-tidy on each file. You can see the setup in .bazelrc
I'm loading .clang-format configuration in .bazelrc

API compatibility

Doesn't break anything, it's just a new Makefile

JIRA Reference

https://swift-nav.atlassian.net/browse/BUILD-360

@wwieclaw wwieclaw requested review from a team and silverjam as code owners January 20, 2023 18:49
@wwieclaw
Copy link
Contributor Author

wwieclaw commented Jan 20, 2023

@silverjam @isaactorz using clang-format with bazel doesn't work for me here:

.clang-format:96:5: error: unknown key 'Delimiter'
  - Delimiter:       pb

Delimiter has been replaced with Delimiters in newer versions of clang and we are using clang-format 14 in bazel.

Is it okay for me to update the libsbp's config?

@jungleraptor
Copy link
Contributor

@silverjam @isaactorz using clang-format with bazel doesn't work for me here:

.clang-format:96:5: error: unknown key 'Delimiter'
  - Delimiter:       pb

Delimiter has been replaced with Delimiters in newer versions of clang and we are using clang-format 14 in bazel.

Is it okay for me to update the libsbp's config?

I don't think so, that will break formatting in CI I'm pretty sure.

Several months ago now we went and updated all of the codebase to using clang-format-14 and clang-tidy-14 but we overlooked this repo because formatting is set up a little differently than the rest. We also need to update the formatter in the base image to get it going in CI.

Let's punt on enabling formatting w/ bazel in this project for right now.

@wwieclaw wwieclaw closed this Jan 23, 2023
@wwieclaw wwieclaw reopened this Jan 23, 2023
@wwieclaw
Copy link
Contributor Author

I removed clang-format related configuration to avoid any confusion

@wwieclaw wwieclaw requested a review from silverjam January 23, 2023 13:22
Copy link
Contributor

@jungleraptor jungleraptor left a comment

Choose a reason for hiding this comment

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

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants