Tinder / bazel-diff

Performs Bazel Target Diffing between two revisions in Git, allowing for Test Target Selection and Selective Building
Other
405 stars 60 forks source link

--useCquery probably incorrectly handles SOURCE_FILE's #218

Closed Asdprom closed 1 month ago

Asdprom commented 5 months ago

Hi!

I noticed a difference in behavior between the cquery and query implementations - on the same repository (bazel-diff-sample.zip), we get different results.

The example is quite simple: target B depends on the source file A.cc through the sample_repo local repository and target A.

//:B -> @sample_repo//:A -> @sample_repo//:A.cc

Query returns significantly more targets than cquery (with the --fineGrainedHashExternalRepos=sample_repo flag specified in both cases), when A.cc is modified.

cquery:

//sample_repo:A

query:

//sample_repo:A
@sample_repo//:A.cc
@sample_repo//:A
//sample_repo:A.cc
//:B

It is possible that the tool with the --useCquery flag is not working correctly. My suspicion falls on these two (1, 2) code fragments, where almost all sources are implicitly filtered out from the target list because SOURCE_FILE has empty rule.name. Which leads to warnings like:

[Warning] Unable to calculate digest for input @sample_repo//:A.cc for rule @sample_repo//:A

And there is no similar filtration performed when the tool is run in query mode. Sorry in advance if I somehow misunderstood this code or idea behind it, but results seem a little bit strange.

honnix commented 2 months ago

Maybe due to https://github.com/Tinder/bazel-diff/blob/eedaebae603be93cb15663dd66d34aa765a1a9b9/cli/src/main/kotlin/com/bazel_diff/bazel/BazelQueryService.kt#L93-L96, which looks like by design.

honnix commented 1 month ago

@tinder-maxwellelliott I think this might be indeed an issue. Not only source files, but also generated files are filter out because of empty rule.name.

honnix commented 1 month ago

I think we might just convert Build.Target into BazelTarget already in BazelQueryService so we can call .name. But would that break the initial design (BazelQueryService should strictly dealing with Bazel proto objects)?

tinder-maxwellelliott commented 1 month ago

@honnix Id be open to a PR for this to see what the impact is

honnix commented 1 month ago

I put up something quickly in #251