Update Public API - #281
Conversation
Szelethus
left a comment
Szelethus
left a comment
There was a problem hiding this comment.
Can we create an issue first as per the PR requirements? It would help us get a clearer picture for the motivation.
- Export get_platform_alias() from defs.bzl
That, alongside codechecker_suite is poorly documented (related: #44). Also, is this something that should be a public feature of this specific ruleset? Can you expand on the motivation?
- Move buildifier to test/buildifier
Does this change have to land within this PR? Sounds like a separate addition worhty of a separate discussion.
| @@ -192,7 +192,7 @@ def _per_file_impl(ctx): | |||
| _create_wrapper_script(ctx, options, compile_commands, config_file) | |||
|
|
|||
| # TODO: Consider using aliases so we don't have to type //src: everywhere. | |||
There was a problem hiding this comment.
There is no need to preserve this comment if this PR lands.
There was a problem hiding this comment.
The toolchain name should reflect the fact that it is a local-only toolchain.
I don't have strong opinions on the placement of the toolchain_type, but we should move it as little as possible. Looking at similar rules, their toolchains are usually like:
- rules_python:
@rules_python//python:toolchain_type - rules_go:
@rules_go//go:toolchain_type - rules_cc:
@rules_cc//cc:toolchain_type - rules_rust:
@rules_rust//rust:toolchain_type
It is my understanding that these rules use the rulename directory as src, so we do not have to follow their pattern.
Remember that this is an API breaking change.
| @@ -0,0 +1,39 @@ | |||
| # Copyright 2026 Ericsson AB | |||
There was a problem hiding this comment.
Moving Buildifier into testing is outside the scope of a Public API patch.
There was a problem hiding this comment.
Agree, I've created a separate PR: #289
|
|
||
| # Tools found on PATH, provisioned by the default_codechecker_tools extension | ||
| codechecker_toolchain( | ||
| name = "default_tools", |
There was a problem hiding this comment.
This name does not communicate that this toolchain cannot be used with remote executors!
This toolchain is specific to a user's local environment.
We could rename it to something like this.
| name = "default_tools", | |
| name = "default_local_tools", |
There was a problem hiding this comment.
Well... "local" is actually kinda misleading and wrong here.
Local to what? Besides, should this be default forever?
Default tools are being taken from the environment currently, which is totally wrong!
Later default_tools should became real default, not local.
Sure, it is a bit late because #269 is already merged,
Very simple - it is being used in real project.
Yes, agree, this might be better separate. |
Disagree. I have explained that - the fact that our current implementation is "local" should not be reflected in the interface.
That was exactly what I checked and exactly my thinking! So, here is the suggestion:
That's why we need to do that as early as possible! |
3 participants
Why:
Still we have to reach into the implementation (//src:) for the toolchain:
@rules_codechecker//src:toolchain_type when declaring own toolchain,
and //src:codechecker_local_toolchain when registering ours.
get_platform_alias() was not reachable from defs.bzl either,
so the public API was incomplete.
What:
codechecker_local_toolchain to default_toolchain