Proof of concept llvm-toolchain

[furtib]

Why:
We want Bazel to handle all dependencies, including the LLVM toolchain.

What:

• Added the LLVM toolchain as a dependency
• Modified the monolithic/single job codechecker rule to use the provided binaries by adding the folder containing them to the PATH

Addresses:
none

Note:
Well that broke spectacularly.....
Issues:

• Each project, including the rule set, will be required to also import the LLVM toolchain (the LLVM toolchain does not allow dependencies to download it) -> This breaks Foss tests and external unit tests,
• Couple legacy tests are failing, will look into it when I have time, some fails due to clang taking precedence over gcc, changing the compile_commands.json content

"llvm_toolchain_llvm" contains every binary, not just clang. I might be mistaken in how I import the toolchain, but I believe we need this one for sure.

[nettle]

Thanks @furtib! This is really interesting!
So... as you mentioned we cannot use this approach at least explicitly because C/C++ rules are trying to use this LLVM toolchain :)
We should think how to move forward

[nettle]

I dont understand this part yet :)

[furtib]

No wonder I for sure made this wrong.
My goal was to have the directory containing the LLVM libraries in PATH.
The documentation for the toolchain did not hold my hand at all on how this should be done.
I'm looking at their tests right now to figure out how they did it.

[furtib]

Well, I made some adjustments, mainly to how the toolchain is recognized.

The new idea is to have an optional parameter where users can provide their llvm_toolchain bin directory as input, and add that to PATH. (This is much better than the one I did before, but it does require more setup from users.)

Does this approach seem reasonable?
I do recognize that defining the llvm_toolchain for each target is not the most ergonomic, but it is the most flexible way. We have seen similar methods in aspect lint. This way, we even support custom LLVM rules.

I believe this would also allow users to de-prioritize the LLVM toolchain in the selection order of C++ toolchains(?).
I'm not sure.

format_test(
    name = "format_test",
    starlark = "@buildifier_prebuilt//:buildifier", # <- this line
	...
)

What still needs to be done:

• For a patch like this to land, we would need to change our testing. (The tests could have their own MODULE.bazel, an example is the llvm_toolchain repo) This is necessary since llvm_toolchain does not allow itself to be a transitive dependency (due to its size). -> This would fix foss tests, and external tests (these are the ones that fail)

[furtib]

To make it a bit more serious, I fixed the tests.
It does make the patch very messy :/

The highlights:

• I copied the testing solution of the llvm_toolchain, so:
  • Tests area separate bazel project, with its own MODULE.bazel. (I did have to modify every single test to accompany this :( )
• I modified the codechecker_pass target in legacy tests to show how users would use the llvm_toolchain. (This is just for demonstration)

[nettle]

The new idea is to have an optional parameter where users can provide their llvm_toolchain bin directory as input, and add that to PATH. (This is much better than the one I did before, but it does require more setup from users.)

Does this approach seem reasonable? I do recognize that defining the llvm_toolchain for each target is not the most ergonomic, but it is the most flexible way. We have seen similar methods in aspect lint. This way, we even support custom LLVM rules.

I believe this would also allow users to de-prioritize the LLVM toolchain in the selection order of C++ toolchains(?). I'm not sure.

Yes, this might be used. Basically this way we can provide path to particular binaries.
And I think this should be at least an option.
However, as I can understand, as soon as we use LLVM toolchain the whole package may want to use it for cc_binary, cc_libary etc. In some cases this is not the scenario users may want.

What still needs to be done:

• For a patch like this to land, we would need to change our testing. (The tests could have their own MODULE.bazel, an example is the llvm_toolchain repo) This is necessary since llvm_toolchain does not allow itself to be a transitive dependency (due to its size). -> This would fix foss tests, and external tests (these are the ones that fail)

I dont think creating more packages inside packages is a good idea. At least it is highly discouraged in Bazel.
Besides, what is the purpose?
To be honest I did not get "llvm_toolchain does not allow itself to be a transitive dependency" etc.
I think we need a meeting where you explain.

[nettle]

To make it a bit more serious, I fixed the tests. It does make the patch very messy :/

Well, the recent change made it impossible to review :)
I guess we should probably drop this...

The highlights:

• I copied the testing solution of the llvm_toolchain, so:

  • Tests area separate bazel project, with its own MODULE.bazel. (I did have to modify every single test to accompany this :( )

We should definitely avoid packages inside packages.
In general the idea of multiple packages is not good and often an indication of bad design.
I saw projects with similar mistakes - later it took much efforts to fix them.

• I modified the codechecker_pass target in legacy tests to show how users would use the llvm_toolchain. (This is just for demonstration)

Yes, I got the idea, Thanks!

[furtib]

Well, the recent change made it impossible to review :)

Sorry about that, I reverted those changes

To be honest I did not get "llvm_toolchain does not allow itself to be a transitive dependency" etc.

This is the offending line:
https://github.com/bazel-contrib/toolchains_llvm/blob/e15a648c23a6305c58f2cb54486ce99fe331696e/toolchain/extensions/llvm.bzl#L55

Not sure what an acceptable way to handle this is.
Lets discuss it in details in a meeting.

(I did make some miscellaneous changes, compared the issue of transitive dependencies it is not important)