Use the compile_commands implementation from the monolithic rule in the per_file rule

[furtib]

Why:
Currently, we use a different, inferior implementation for generating compile_commands.json in the pre_file rule.
We should use the same as in the monolithic rule. This would prevent us from having to maintain both rules simultaneously.

What:
Replaces the compile_commands.json generating implementation in the pre_file rule.
Fixes the unittest for compile_commands, the per_file compile_commands.json location changed

Addresses:
Fixes: #120
Fixes: #128

[furtib]

Fails because #99 does not affect the "monolithic" compile command generator implementation. should rebase after merging #100

[Szelethus]

Can you delete all unnecessary functions, just to see how much simpler this solution would be?

[furtib]

This patch might deserve a bit more love.
The file collection still happens through the aspect written inside the per file rule.
We might want to use the solution from compile_commands.bzl there too, the fix I have implemented this has been removed (we concentrated on the monolithic rule for the time being, the patch was: #127), but I think that could have placed here.

[furtib]

Using the compile_commands.bzl aspect for file collection, there are some things to keep in mind. Mainly, despite the name transitive_source_files suggests only having source files, it actually can contain header files (this might be because it collects the files under the srcs=[] part of the cc_lib/bin rules)

[Szelethus]

Just like that, and everything runs? Damn.

[Szelethus]

We need to understand this a bit better before merging...

[nettle]

May I suggest changing only src/per_file.bzl implementation first, which seems reasonable to me and not touching src/codechecker.bzl and src/compile_commands.bzl at least yet.

[Szelethus]

May I suggest changing only src/per_file.bzl implementation first, which seems reasonable to me and not touching src/codechecker.bzl and src/compile_commands.bzl at least yet.

It is a more than reasonable ask to land parts of this patch independently, but I'm strongly against not touching those two files to keep them stable, especially with NFC patches.

[Szelethus]

Meant to send this inline comment yesterday.

[nettle]

Looks generally good to me, with a couple of comments:

1. To be honest I dont understand the hustle with headers, but I guess you have your reasons, maybe just need to explain, maybe try to implement as a separate function for clarity
2. Nitpick: the coding style is quite questionable :) but we do not run buildifier yet.
   I would probably suggest to start it running to fix ONLY changed parts :)

[nettle]

We can make it much simpler

[Szelethus]

LGTM, I think we should merge this and move on.

I'd be happier if I got a response explaining why #141 or #145 couldn't land as a precursor. Writing DRY code is a basic software engineering practice, so explicitly deciding against it requires some justification. I was (and am) willing to let the issue slide, but I want to encourage constructive, technical discussion on such topics in the future; what are the technical reasons behind the obstruction?

I am happy to approve something I disagree with, but not something that I don't even understand.

[nettle]

LGTM