Skip to content

Conversation

@furtib
Copy link
Contributor

@furtib furtib commented Sep 15, 2025

Why:
The inline bash script is very limiting.

What:
Rewrote the bash inline script into a separate Python file.

Adresses:

@furtib furtib self-assigned this Sep 15, 2025
@furtib furtib added the non-functional change ☮️ The patch doesn't change any functionality, e.g. refactoring, documentation, test-only. label Sep 15, 2025
@furtib furtib requested a review from Szelethus September 15, 2025 10:14
@furtib furtib force-pushed the rewrite_code_checker_to_python branch 2 times, most recently from 680a6b8 to 534b527 Compare September 22, 2025 08:36
@furtib furtib force-pushed the rewrite_code_checker_to_python branch 2 times, most recently from cf8658c to 57d6599 Compare September 29, 2025 11:18
Copy link
Contributor

@Szelethus Szelethus left a comment

Choose a reason for hiding this comment

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

I just opened this file right next to src/codechecker_script.py in a terminal. There are some striking similarities:

  • You declare a log() and a _display_error() function, but there are already handy, and possibly more mature log functions in the other script: fail(), stage(), input_data().
  • _run_codechecker() competes with execute(), prepare, analyze()
  • Both _move_plist_files() and functions realpath(), resolve_plist_symlinks(), resolve_yaml_symlinks(), resolve_symlinks(), update_file_paths() are about results postprocessing, although they do it differently.

Some other questions arise when you consider the diffrences:

  • Did we ever understand how do we relativize the paths in the plist files that come from the per-file rule? It seems to be only implemented in the monolithic rule.
  • The per_file rule defines _create_compile_commands_json_with_absolute_paths(), but I didn't immediately see anything similar for the other one. Why?

I am perfectly aware of the fact that I'm pointing out things which are specifically beyond the scope of your patch. Still, I'd at least like to see how these things would ideally look like, even if we don't solve them right away.

@furtib
Copy link
Contributor Author

furtib commented Oct 6, 2025

  • In the case of log functions, I agree that the ones in codechecker_script.py are more mature, but I don't want to include from it; I would rather create a common library and include from that in both files. (Out of scope for this PR) So the current implementation can be regarded as a placeholder. (Also, including will be a challenge in itself since, at the moment, only one script is present during the Bazel execution.)

  • In case of the execute, prepare, and so on, it would still be my ideal option to create a common library from these. These functions will have to be modified to handle the per-file rule properly. For example, the hardcoded location of the BAZEL_BIN is not compatible with remote execution, which is the main selling point of the per_file rule.

  • In the case of the _move_plist_files(), this step is completely unrelated to what the other post-processing steps do.
    This function moves the plist files to their Bazel-declared path; it doesn't touch their contents. (This also means that for the time being (just as in the bash script), no path gets relativized, or resolved (they appear that way locally because of Codechecker) ) This function is only necessary for the per-file rule. The functions realpath(), etc., are modifying the content of the files, not their locations.

  • _create_compile_commands_json_with_absolute_paths() exits because of this line in the bash script:

    sed 's|"directory":"."|"directory":"'$(pwd)'"|g' $COMPILE_COMMANDS_JSON > $COMPILE_COMMANDS_ABS

    I'm not sure why it's only present in the per-file rule. I think it might be due to the different analyze commands may run in different directories (remote execution places a hash in the folder name).

@furtib furtib force-pushed the rewrite_code_checker_to_python branch from 65ea5e1 to bf07dfd Compare October 7, 2025 09:00
Copy link
Contributor

@Szelethus Szelethus left a comment

Choose a reason for hiding this comment

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

Okay, lets not get overambitious with this patch, I like the overall direction.

@furtib furtib requested a review from Szelethus October 10, 2025 11:09
furtib and others added 2 commits October 13, 2025 13:46
Co-authored-by: Kristóf Umann <dkszelethus@gmail.com>
@furtib furtib force-pushed the rewrite_code_checker_to_python branch from 2d7c437 to f988dd4 Compare October 13, 2025 12:18
@furtib furtib requested a review from Szelethus October 13, 2025 12:21
Copy link
Contributor

@Szelethus Szelethus left a comment

Choose a reason for hiding this comment

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

LGTM. There are miniscule things we could fuss about for weeks if we really wanted to, but the PR already provides more value and improvement than issues to fix. And honestly, those issues or cosmetic things are better fixed in a followup patch.

@furtib furtib merged commit 87e30ab into Ericsson:main Oct 14, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

non-functional change ☮️ The patch doesn't change any functionality, e.g. refactoring, documentation, test-only.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants