Skip to content

Merge CodeChecker default and user defined arguments #88

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
furtib wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Merge CodeChecker default and user defined arguments #88

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4405ad7
Add function to merge default and user defined arguments together
furtib Sep 16, 2025
a1fff85
Make merge options more talkative
furtib Oct 13, 2025
750fee7
Enable test
furtib Oct 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion src/per_file.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,10 +314,27 @@ def _collect_all_sources_and_headers(ctx):
sources_and_headers = all_files + headers.to_list()
return sources_and_headers

def _get_argument_name(argument):
return argument.split(" ")[0].split("=")[0]

def _merge_options(default, custom):
"""
Merge command line arguments so that default options can be overridden
"""
final = []
args_set = []
for item in custom:
args_set.append(_get_argument_name(item))
final.append(item)
for option in default:
if _get_argument_name(option) not in args_set:
final.append(option)
return final
Comment on lines +320 to +332
Copy link
Copy Markdown
Contributor

@Szelethus Szelethus Sep 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we do something more elegant? Are depsets appropriate here, which can leverage uniquing?

Copy link
Copy Markdown
Contributor Author

@furtib furtib Sep 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depsets are not appropriate here, since what I really want to solve is "--analyzers" showing up twice if the user defines it too.

The reason to do this at all is to be able to know which analyzers will run, and thus what output plist files I need to declare.

Since the whole argument is as: "--analyzers clangsa clang-tidy" and a user-defined could be "--analyzers cppcheck", these two strings are different, so they would both go into the depset. We could add only the first part "--analyzers" to the depset; this way, it would clear the duplicate, but how do we know what the list of activated analyzers is?

We could restrict the merge to only consider "--analyzers" arguments, if that would be more elegant, and leave other parameters alone.

Copy link
Copy Markdown
Contributor Author

@furtib furtib Oct 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just noticed that a user defining an argument multiple times could cause problems.

Copy link
Copy Markdown
Contributor

@Szelethus Szelethus Oct 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meaning enabling multiple checkers like -e cplusplus -e core etc?

Copy link
Copy Markdown
Contributor Author

@furtib furtib Oct 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would actually be fine. I mean my goal with this was to have at most a single --analyzer argument, so I can use its content easily to determine how many output files I should declare in Bazel. But if the user is able to declare multiple --analyzer arguments, I'm still in the dark.

Copy link
Copy Markdown
Contributor

@Szelethus Szelethus Oct 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, the last --analyzers argument will be used (which is a common technique to make scripting, and appending easier, just need to append some new flags at the end).

Copy link
Copy Markdown
Contributor

@Szelethus Szelethus Oct 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, the previous loop preserves all user declared options, right? Maybe its not a good omen to mess with that, even if it contains multiple flags of which only the last will be used. Is this a big issue for you?

Copy link
Copy Markdown
Contributor Author

@furtib furtib Oct 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not a huge issue, just makes this patch basically pointless.


def _per_file_impl(ctx):
compile_commands_json = _compile_commands_impl(ctx)
sources_and_headers = _collect_all_sources_and_headers(ctx)
options = ctx.attr.default_options + ctx.attr.options
options = _merge_options(ctx.attr.default_options, ctx.attr.options)
all_files = [compile_commands_json]
for target in ctx.attr.targets:
if not CcInfo in target:
Expand Down
4 changes: 1 addition & 3 deletions test/unit/argument_merge/test_argument_merge.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,7 @@ def test_per_file_argument_merge(self):
+ "test-unit-argument_merge-main.cc_codechecker.log",
r"--analyzers",
)
# FIXME: Change to '1'
# Should only have this argument set once
self.assertEqual(len(re.findall("--analyzers", matched_lines[0])), 2)
self.assertEqual(len(re.findall("--analyzers", matched_lines[0])), 1)


if __name__ == "__main__":
Expand Down