Use the compile_commands implementation from the monolithic rule in the per_file rule #98

furtib · 2025-10-21T11:41:07Z

Why:
Currently, we use a different, inferior implementation for generating compile_commands.json in the pre_file rule.
We should use the same as in the monolithic rule. This would prevent us from having to maintain both rules simultaneously.

What:
Replaces the compile_commands.json generating implementation in the pre_file rule.
Fixes the unittest for compile_commands, the per_file compile_commands.json location changed

Addresses:
Fixes: #120
Fixes: #128

furtib · 2025-11-24T12:47:18Z

Fails because #99 does not affect the "monolithic" compile command generator implementation. should rebase after merging #100

Szelethus

Can you delete all unnecessary functions, just to see how much simpler this solution would be?

furtib · 2026-01-10T22:16:12Z

This patch might deserve a bit more love.
The file collection still happens through the aspect written inside the per file rule.
We might want to use the solution from compile_commands.bzl there too, the fix I have implemented this has been removed (we concentrated on the monolithic rule for the time being, the patch was: #127), but I think that could have placed here.

furtib · 2026-01-12T07:30:03Z

Using the compile_commands.bzl aspect for file collection, there are some things to keep in mind. Mainly, despite the name transitive_source_files suggests only having source files, it actually can contain header files (this might be because it collects the files under the srcs=[] part of the cc_lib/bin rules)

Szelethus

Just like that, and everything runs? Damn.

src/per_file.bzl

Szelethus · 2026-01-12T08:53:08Z

src/per_file.bzl

+                    if not check_valid_file_type(src):
+                        continue


When does this happen?

Mainly, despite the name transitive_source_files suggests only having source files, it actually can contain header files (this might be because it collects the files under the srcs=[] part of the cc_lib/bin rules)

…he compile_commands solution transitively fixes the per_file rule

…e_commands.bzl fixes it transitively

…assembly

Szelethus

We need to understand this a bit better before merging...

Szelethus · 2026-01-12T12:20:40Z

src/per_file.bzl

+                for elem in headers_mid:
+                    if type(elem) == "depset":
+                        headers.extend(elem.to_list())
+                    else:
+                        headers.append(elem)


Do we have to do this for the monolithic version? If not, why not?

Szelethus · 2026-01-12T12:21:28Z

src/per_file.bzl

+        trans_depsets = [compilation_context.headers]
+        for dset in target[SourceFilesInfo].headers.to_list():
+            trans_depsets.append(dset)
+        headers = depset(direct = [src],
+            transitive = trans_depsets)


Lets turn this into a function.

nettle · 2026-01-12T20:49:27Z

src/codechecker.bzl

-    # Get compile_commands.json file and source files
-    compile_commands = None
-    source_files = None
-    for output in compile_commands_impl(ctx):
-        if type(output) == "DefaultInfo":
-            compile_commands = output.files.to_list()[0]
-            source_files = output.default_runfiles.files.to_list()
-    if not compile_commands:
-        fail("Failed to generate compile_commands.json file!")
-    if not source_files:
-        fail("Failed to collect source files!")
-    if compile_commands != ctx.outputs.compile_commands:
-        fail("Seems compile_commands.json file is incorrect!")


I would prefer not touching this for compatibility sake

nettle · 2026-01-12T20:54:01Z

src/per_file.bzl

+        trans_depsets = [compilation_context.headers]
+        for dset in target[SourceFilesInfo].headers.to_list():
+            trans_depsets.append(dset)
+        headers = depset(direct = [src],


Not sure I understand this line...
So, this is not just headers but src as well?

nettle · 2026-01-12T20:54:46Z

src/per_file.bzl

        # NOTE: we collect only headers, so CTU may not work!
-        headers = depset([src], transitive = [compilation_context.headers])
+        trans_depsets = [compilation_context.headers]
+        for dset in target[SourceFilesInfo].headers.to_list():


I guess dest is actually a header :)

nettle · 2026-01-12T21:02:49Z

src/compile_commands.bzl

        ),
    ]

+def get_compile_commands_json_and_srcs(ctx):


Maybe I'm mistaken, at least I do not see that yet, but I guess adding this function does not give us much benefits:
As I mentioned I would prefer original implementation in src/codechecker.bzl.
And in src/per_file.bzl we even do not need this function.

This is literally copy-pasted from codechecker.bzl, so this is above and beyond NFC. Since the output of compile_commands_impl needs the same handling both in the per-file and monolithic implementations, I think this is very sensible refactoring step.

In terms of merging order, sure, we can land the creating of this convenience function before or after replacing the compile commands generation parts.

nettle

May I suggest changing only src/per_file.bzl implementation first, which seems reasonable to me and not touching src/codechecker.bzl and src/compile_commands.bzl at least yet.

Szelethus · 2026-01-13T14:55:59Z

May I suggest changing only src/per_file.bzl implementation first, which seems reasonable to me and not touching src/codechecker.bzl and src/compile_commands.bzl at least yet.

It is a more than reasonable ask to land parts of this patch independently, but I'm strongly against not touching those two files to keep them stable, especially with NFC patches.

Szelethus

Meant to send this inline comment yesterday.

Szelethus · 2026-01-13T14:43:58Z

src/compile_commands.bzl

        ),
    ]

+def get_compile_commands_json_and_srcs(ctx):


This is literally copy-pasted from codechecker.bzl, so this is above and beyond NFC. Since the output of compile_commands_impl needs the same handling both in the per-file and monolithic implementations, I think this is very sensible refactoring step.

In terms of merging order, sure, we can land the creating of this convenience function before or after replacing the compile commands generation parts.

nettle · 2026-01-16T20:31:45Z

src/per_file.bzl

+    compile_commands_json, source_files = \
+        get_compile_commands_json_and_srcs(ctx)


The following can be used to get compile_commands_json:

for output in compile_commands_impl(ctx): if type(output) == "DefaultInfo": compile_commands_json = output.files.to_list()[0]

furtib requested review from Szelethus and nettle October 21, 2025 11:41

furtib self-assigned this Oct 21, 2025

furtib added enhancement New feature or request non-functional change ☮️ The patch doesn't change any functionality, e.g. refactoring, documentation, test-only. labels Oct 21, 2025

furtib force-pushed the unify-compile_commands branch from ff788bf to c1b559c Compare November 24, 2025 12:34

furtib force-pushed the unify-compile_commands branch from c1b559c to fea15a0 Compare November 24, 2025 13:56

Szelethus reviewed Jan 10, 2026

View reviewed changes

furtib force-pushed the unify-compile_commands branch from fea15a0 to 65034d3 Compare January 10, 2026 22:12

furtib force-pushed the unify-compile_commands branch from 7827b46 to cba2a86 Compare January 12, 2026 07:32

Szelethus requested changes Jan 12, 2026

View reviewed changes

furtib force-pushed the unify-compile_commands branch from 3fb354f to 936ed44 Compare January 12, 2026 11:39

furtib added 3 commits January 12, 2026 12:44

Use the compile_commands implementation from the monolithic rule

a9eb302

Relocated, compile_commands.json, fix the test for it

c2c2686

Remove unused code

571fe46

furtib force-pushed the unify-compile_commands branch from 936ed44 to 4ce7518 Compare January 12, 2026 11:46

furtib and others added 5 commits January 12, 2026 13:09

Use file collection from compile_commands.bzl

592a930

Enable test for per_file implementation_deps virtual headers, using t…

f6ba09f

…he compile_commands solution transitively fixes the per_file rule

Enable test for external repositories for per_file rule, using compil…

b2b9cd0

…e_commands.bzl fixes it transitively

Remove unused loads

f9fe187

Remove code duplication for monolithic and per-file compile commands …

e4b2503

…assembly

furtib force-pushed the unify-compile_commands branch from 4ce7518 to e4b2503 Compare January 12, 2026 12:09

Szelethus reviewed Jan 12, 2026

View reviewed changes

nettle reviewed Jan 12, 2026

View reviewed changes

nettle requested changes Jan 12, 2026

View reviewed changes

Szelethus reviewed Jan 14, 2026

View reviewed changes

Szelethus mentioned this pull request Jan 15, 2026

Greate a convenience function for interacting with compile_commands_impl #141

Open

nettle mentioned this pull request Jan 16, 2026

Convenience function for interacting with compile_commands_impl #145

Open

nettle requested changes Jan 16, 2026

View reviewed changes

		compile_commands_json, source_files = \
		get_compile_commands_json_and_srcs(ctx)

Use the compile_commands implementation from the monolithic rule in the per_file rule #98

Are you sure you want to change the base?

Use the compile_commands implementation from the monolithic rule in the per_file rule #98

Conversation

furtib commented Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

furtib commented Nov 24, 2025

Uh oh!

Szelethus left a comment

Choose a reason for hiding this comment

Uh oh!

furtib commented Jan 10, 2026

Uh oh!

furtib commented Jan 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Szelethus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Szelethus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nettle Jan 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nettle left a comment

Choose a reason for hiding this comment

Uh oh!

Szelethus commented Jan 13, 2026

Uh oh!

Szelethus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

furtib commented Oct 21, 2025 •

edited

Loading

furtib commented Jan 12, 2026 •

edited

Loading

nettle Jan 12, 2026 •

edited

Loading