Update rules for clang-tidy and clang --analyze

[nettle]

Why:
Bazel rules for clang-tidy and clang --analyze
were never updated and never tested on real projects.

What:

• Collect sources and headers in transitive dependencies
• Support platform transitions (for multi platform projects)
• Simplify launcher scripts, bazelify parameters
• Fix plist generation for clang --analyze
• Support config file to accept input from a file_group
• Remove clang-tidy aspect since it is not actually used
• Minor cosmetic changes
• Test on some of real Ericsson projects
• Updated README

[Szelethus]

There are plenty of things happening in this patch. Both the changes and the summary feels like "spring cleaning", sweeping the floors and hammering some nails flush if they wiggled themselves out, and also fixing that wall plug that hasn't worked for years.

Can you give me a bulletpoint list that stated what happens and why? From what I gather

• Bash script paramaters have been bazelized,
• Fixing a minor issue with config file inputs when they are from a file group,
• Changing the parameters of the clang call,
• Some code cosmetic update,
• Refactoring the tidy aspect,
• Removing two tests...

And many others. Which parts are NFC, and change for the sake of making the code more readable? Which parts are NFC, but prepare for a later coming feature (like newer bazel versions)? Which parts are functional, as some seem to be functional, and how are they tested?

I approve of most the changes, but the patch in its current state feels like a proof of concept.

[furtib]

I forgot to add a big summary.
I think this patch is good. I don't understand the removal of the aspect. Was it unused?

(Also, we should keep in mind that the README should be updated to reflect the removal of the aspect)

[nettle]

There are plenty of things happening in this patch. Both the changes and the summary feels like "spring cleaning", sweeping the floors and hammering some nails flush if they wiggled themselves out, and also fixing that wall plug that hasn't worked for years.

Can you give me a bulletpoint list that stated what happens and why? From what I gather

• Bash script paramaters have been bazelized,
• Fixing a minor issue with config file inputs when they are from a file group,
• Changing the parameters of the clang call,
• Some code cosmetic update,
• Refactoring the tidy aspect,
• Removing two tests...

And many others. Which parts are NFC, and change for the sake of making the code more readable? Which parts are NFC, but prepare for a later coming feature (like newer bazel versions)? Which parts are functional, as some seem to be functional, and how are they tested?

I approve of most the changes, but the patch in its current state feels like a proof of concept.

Done

[nettle]

I forgot to add a big summary. I think this patch is good. I don't understand the removal of the aspect. Was it unused?

(Also, we should keep in mind that the README should be updated to reflect the removal of the aspect)

Done

[Szelethus]

Thanks for the summary update! As a preface, I agree with the direction and, as much as I understand, with the way you implemented it as well.

I've spent some time with the patch, but I admit that for me, it is simply too big and too complicated to decipher which line changes are attributed to which point in the bulletpoint list. That means I can't do anything beyond a "looks about right" review.

With that said, I'm actually fine with landing the patch as-is, since you are the main maintainer of these rules and I take your word that it satisfies real-world usage needs.

The only issue that remains for me is testing. For the following points:

• Support platform transitions (for multi-platform projects)

I suppose this is borderline impossible to write tests for here.

• Fix plist generation for clang --analyze

What was the specific issue you were experiencing? Can you add tests for it?

• Support config file to accept input from a file_group

Can we add a test for this?

• Collect sources and headers in transitive dependencies
• Simplify launcher scripts, bazelify parameters
• Minor cosmetic changes
• Updated README

Do I understand correctly that all of these are NFC changes?

With tests added and the NFC changes clarified, the patch is ready to land.

[nettle]

With that said, I'm actually fine with landing the patch as-is, since you are the main maintainer of these rules and I take your word that it satisfies real-world usage needs.

The only issue that remains for me is testing. For the following points:

Yeah, unfortunately we do not have proper testing for clang_tidy_test and clang_analyze_test.
The main reason is that those are, we can say, experimental and not being used yet.
Can we agree on creating a separate ticket to introduce tests for these tests?
To at least unblock this patch. (The main criteria: is the change improves or degrades?)

• Support platform transitions (for multi-platform projects)

I suppose this is borderline impossible to write tests for here.

Yes, this is not impossible, but looks quite difficult.

• Fix plist generation for clang --analyze

What was the specific issue you were experiencing? Can you add tests for it?

I'm afraid I cannot, but I'll check.
I guess this is project dependent (e.g. multi-platform).

• Support config file to accept input from a file_group

Can we add a test for this?

I think I can. At least this one, but I'm not sure it is essential functionality.
I just borrowed this from a real project.

• Collect sources and headers in transitive dependencies
• Simplify launcher scripts, bazelify parameters
• Minor cosmetic changes
• Updated README

Do I understand correctly that all of these are NFC changes?

Nop, transitive dependencies is essential, not NFC at all.
Multi-platform projects need this.

With tests added and the NFC changes clarified, the patch is ready to land.

How about 1 - merge, 2 - assign task to me to develop and add tests?

[Szelethus]

Yeah, unfortunately we do not have proper testing for clang_tidy_test and clang_analyze_test. The main reason is that those are, we can say, experimental and not being used yet. Can we agree on creating a separate ticket to introduce tests for these tests? To at least unblock this patch. (The main criteria: is the change improves or degrades?)

How about 1 - merge, 2 - assign task to me to develop and add tests?

Sure! Lets hope that other changes won't unknowingly break functionality.

[Szelethus]

Just a small request -- please create the issues for the tests before the merge. Otherwise, proceed at your pace!

[nettle]

Just a small request -- please create the issues for the tests before the merge. Otherwise, proceed at your pace!

Absolutely! 👍
Created #166 :)