Add dependencies of tools from toolchain to runfiles - #274
Conversation
|
Right thinking @furtib! |
| transitive = [ | ||
| # We also collect files necessary for these programs to run. | ||
| # Those files should be declared with `data = [...]` | ||
| # in the executable's target. | ||
| ctx.attr.codechecker[DefaultInfo].data_runfiles.files, | ||
| ctx.attr.clangsa[DefaultInfo].data_runfiles.files, | ||
| ctx.attr.clang_tidy[DefaultInfo].data_runfiles.files, | ||
| ], |
There was a problem hiding this comment.
I just wonder what dependencies we see here?
Can/should we add a test for this?
There was a problem hiding this comment.
CodeChecker is not a single executable, so we mainly expect its files to be here.
I will try to mock something up using skylib.
There was a problem hiding this comment.
My point is - I think it has no sense to add/change something that does not work.
My question is - if this works what exact dependencies we actually missing?
There was a problem hiding this comment.
In the current state of the toolchain, it only captures the entry script of a standalone codechecker tar.gz (CodeChecker/bin/CodeChecker). Any other file (e.g., CodeChecker/bin/subcmd/CodeChecker-main which is called by the entry script) is not captured by the toolchain.
There was a problem hiding this comment.
I think it has no sense to add/change something that does not work.
What do you mean specifically?
There was a problem hiding this comment.
So, do you see CodeChecker/bin/subcmd/CodeChecker-main in transitive dependencies? To be honest I have some doubts that transitive works here, it would be really interesting to see...
| config_file, | ||
| ] + source_files, | ||
| ), | ||
| tools = [info.runfiles], |
1ddc6cc to
0e9307e
Compare
Szelethus
left a comment
Szelethus
left a comment
There was a problem hiding this comment.
I understand that we are following standard Bazel idioms here, but they might not be that obvious, especially how we expect the data field of the toolchain to be populated. Lets document how to add a custom toolchain in README.
I know that we will need to split README up eventually -- but until thats done, throw it there.
| transitive = [ | ||
| # We also collect files necessary for these programs to run. | ||
| # Those files should be declared with `data = [...]` | ||
| # in the executable's target. | ||
| ctx.attr.codechecker[DefaultInfo].data_runfiles.files, | ||
| ctx.attr.clangsa[DefaultInfo].data_runfiles.files, | ||
| ctx.attr.clang_tidy[DefaultInfo].data_runfiles.files, | ||
| ], |
There was a problem hiding this comment.
I think it has no sense to add/change something that does not work.
What do you mean specifically?
Szelethus
left a comment
Szelethus
left a comment
There was a problem hiding this comment.
I know this is a cheap review, but please create an issue. You can likely reproduce with CodeChecker's standalone_package target.
3 participants
Why:
We currently only provide the single entry-script/binary to tools defined with our toolchain. (In CodeCheckers' case, this means only the entry script is available in the sandbox, not the entire installation of CodeChecker.)
This can cause trouble when users are trying to use the toolchain with their custom Bazel-provided CodeChecker versions.
What:
Addresses:
none
Test: #277