Feat: Add option to override toolchain on a per target basis - #294
Feat: Add option to override toolchain on a per target basis#294furtib wants to merge 3 commits into
Conversation
furtib
changed the title
49eeec8 to
8c383e1
Compare
| config_file, codechecker_env = get_config_file(ctx) | ||
|
|
||
| info = ctx.toolchains["//src:toolchain_type"].codecheckerinfo | ||
| info = resolve_toolchain_info(ctx) |
There was a problem hiding this comment.
Kind of doubt that we need a function here...
if ctx.attr.toolchain:
info = ctx.attr.toolchain[platform_common.ToolchainInfo].codecheckerinfo
else:
info = ctx.toolchains["//:toolchain_type"].codecheckerinfoThere was a problem hiding this comment.
I wanted to avoid code duplication as much as possible (we have to do this for all 3 rules: codechecker, codechecker_test and pre_file_test), but I guess we do not necessarily need it.
There was a problem hiding this comment.
What is the drawback behind small, tidy functions that are reused? I'd strongly prefer the earlier solution.
There was a problem hiding this comment.
The main drawback of small functions is that excessive breaking up of code increases cognitive overhead and makes it harder to follow the overall logic. The same for splitting functionality into multiple files - that increases navigation complexity, which forces you to jump between files just to trace how a feature works. Besides it makes solution less self-contained and tend to add unnecessary dependencies (tight coupling) which also leads to loosing track of them - we already have it (tools.bzl, common.bzl, codechecker_toolchain.bzl). Ultimately, a balanced and very reasonable approach should be taken when breaking down code into functions and files, and not just DRY.
furtib
left a comment
furtib
left a comment
There was a problem hiding this comment.
Thank you for the review!
| config_file, codechecker_env = get_config_file(ctx) | ||
|
|
||
| info = ctx.toolchains["//src:toolchain_type"].codecheckerinfo | ||
| info = resolve_toolchain_info(ctx) |
There was a problem hiding this comment.
I wanted to avoid code duplication as much as possible (we have to do this for all 3 rules: codechecker, codechecker_test and pre_file_test), but I guess we do not necessarily need it.
f4a947e to
0c119c9
Compare
3 participants
Why:
Users want the ability to use different versions of CodeChecker (or analyzers) on a per-target basis.
What:
codechecker_toolchainparameter tocodechecker_test, through which users can override Bazel's toolchain resolution. (Preserves previous behaviour if not specified.)Addresses:
Fixes: #278