[PATCH] D108366: [clang][deps] Deduce resource directory from the compiler path

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith requested changes to this revision.
dexonsmith added a comment.
This revision now requires changes to proceed.

In D108366#2959736 <https://reviews.llvm.org/D108366#2959736>, @jansvoboda11 wrote:

> For `clang-scan-deps`, this is //almost// NFC. This code kicks in iff `ResourceDirectoryCache` doesn't provide any result:
>
> - the execution of `CommandLine[0] -print-resource-dir` command returns a non-zero exit code (exercised in the test) or doesn't print anything,
> - the `CommandLine` is empty,
> - the compiler executable (`CommandLine[0]`) is not an absolute path.
>
> That's why I want to check with people if we can agree on removing `ResourceDirectoryCache`. I'd be keen on updating this patch to include the change and make the test more obviously correct.

Got it.

I wonder if it'd be better to split "changing default behaviour of clang-scan-deps the tool" from "avoid inject-resource-dir logic when it's incorrect". I.e., commit something in this patch to fix the immediate problem and remove/optimize ResourceDirectoryCache in a follow-up.

For this patch, you could add an inject-resource-dir flag to the scanning service, which controls the new Clang::Tooling flag (and can be controlled by libclang), and a command-line flag in clang-scan-deps to allow testing both sides of it. Would be worth documenting the testcase with a good comment to explain the corner case. Maybe the command-line flag would also disable ResourceDirectoryCache.



================
Comment at: clang/test/ClangScanDeps/resource_directory.c:3
+// RUN: cp %S/Inputs/resource_directory/* %t
+// RUN: sed -e "s|CLANG|/our/custom/bin/clang|g" -e "s|DIR|%/t|g" %S/Inputs/resource_directory/cdb_tu.json > %t/cdb.json
+
----------------
I don't love this hardcoded path to clang which //could// exist on some machine.

Could you use `/dev/null` for the path to clang?
Or `/dev/null/bin/clang`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108366/new/

https://reviews.llvm.org/D108366