[clang-include-cleaner] Fix incorrect directory issue for writing files #111375

bc-lee · 2024-10-07T13:22:08Z

If the current working directory of clang-include-cleaner differs from the directory of the input files specified in the compilation database, it doesn't adjust the input file paths properly. As a result, clang-include-cleaner either writes files to the wrong directory or fails to write files altogether.

This pull request fixes the issue by adjusting the input file paths based on the directory specified in the compilation database. If that directory is not writable, clang-include-cleaner will write the output relative to the current working directory.

Fixes #110843.

If the current working directory of `clang-include-cleaner` differs from the directory of the input files specified in the compilation database, it doesn't adjust the input file paths properly. As a result, `clang-include-cleaner` either writes files to the wrong directory or fails to write files altogether. This pull request fixes the issue by checking whether the input file path is absolute. If it isn't, the path is concatenated with the directory of the input file specified in the compilation database. Fixes llvm#110843.

llvmbot · 2024-10-07T13:22:42Z

@llvm/pr-subscribers-clang-tools-extra

Author: Byoungchan Lee (bc-lee)

Changes

If the current working directory of clang-include-cleaner differs from the directory of the input files specified in the compilation database, it doesn't adjust the input file paths properly. As a result, clang-include-cleaner either writes files to the wrong directory or fails to write files altogether.

This pull request fixes the issue by checking whether the input file path is absolute. If it isn't, the path is concatenated with the directory of the input file specified in the compilation database.

Fixes #110843.

Full diff: https://github.com/llvm/llvm-project/pull/111375.diff

1 Files Affected:

(modified) clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp (+22-2)

diff --git a/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp b/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp
index 080099adc9a07a..1cb8b9c4ae99b7 100644
--- a/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp
+++ b/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp
@@ -173,11 +173,22 @@ class Action : public clang::ASTFrontendAction {
     if (!HTMLReportPath.empty())
       writeHTML();
 
+    // Source File's path relative to compilation database.
     llvm::StringRef Path =
         SM.getFileEntryRefForID(SM.getMainFileID())->getName();
     assert(!Path.empty() && "Main file path not known?");
     llvm::StringRef Code = SM.getBufferData(SM.getMainFileID());
 
+    auto Cwd = getCompilerInstance()
+                   .getFileManager()
+                   .getVirtualFileSystem()
+                   .getCurrentWorkingDirectory();
+    if (Cwd.getError()) {
+      llvm::errs() << "Failed to get current working directory: "
+                   << Cwd.getError().message() << "\n";
+      return;
+    }
+
     auto Results =
         analyze(AST.Roots, PP.MacroReferences, PP.Includes, &PI,
                 getCompilerInstance().getPreprocessor(), HeaderFilter);
@@ -201,8 +212,17 @@ class Action : public clang::ASTFrontendAction {
       }
     }
 
-    if (!Results.Missing.empty() || !Results.Unused.empty())
-      EditedFiles.try_emplace(Path, Final);
+    if (!Results.Missing.empty() || !Results.Unused.empty()) {
+      auto Sept = llvm::sys::path::get_separator();
+      // Adjust the path to be absolute if it is not.
+      std::string FullPath;
+      if (llvm::sys::path::is_absolute(Path))
+        FullPath = Path.str();
+      else
+        FullPath = Cwd.get() + Sept.str() + Path.str();
+
+      EditedFiles.try_emplace(FullPath, Final);
+    }
   }
 
   void writeHTML() {

bc-lee · 2024-10-07T22:15:46Z

@kadircet @hokein Could you please review this PR? It is related to #102615. More details are provided in the related issue. Thanks!

kadircet

thanks a lot for the patch, and sorry for regressing this.

I think this is also going to regress behavior in some cases (e.g. working directory in compile commands might be virtual for some build systems like bazel).

When include-cleaner is invoked as clang-include-cleaner foo.cc the user is definitely trying to run this on a file relative to directory in which we invoked include-cleaner, hence we should make sure it's adressed through that path going forward.

Can we instead update logic near the following in main:

 clang::tooling::ClangTool Tool(OptionsParser->getCompilations(),
                                 OptionsParser->getSourcePathList());

instead of passing OptionsParser->getSourcePathList()), we can turn all of these source paths into absolue paths, using the current-working-dir of the process, and then we don't need to worry about how they make their way into our ast-consumer.

bc-lee · 2024-10-09T01:41:18Z

Could you provide a minimal reproducer for this case if possible? Ideally, it should not depend on Google's internal Bazel setup or tools. From what I understand, Bazel itself doesn’t generate compilation databases, and third-party tools adjust the paths of raw compiler invocations to the correct ones. I tested two methods for generating compilation databases for Bazel projects in my demo repository (https://github.com/bc-lee/test-compilation-database), and both methods correctly generated the compilation databases, with no issues traversing over the Bazel build directory.

For instance, when I ran kiron1/bazel-compile-commands in my demo repository, it generated the following compile_commands.json:

Click to toggle

[
  {
    "directory": "/home/leebc/.cache/bazel/_bazel_leebc/5ea2deb937e75f0bf29ba4c49931e67d/execroot/_main",
    "command": "/usr/bin/gcc -U_FORTIFY_SOURCE -fstack-protector -Wall -Wunused-but-set-parameter -Wno-free-nonheap-object -fno-omit-frame-pointer -std=c++14 -MD -MF bazel-out/k8-fastbuild/bin/api/_objs/main/main.pic.d -frandom-seed=bazel-out/k8-fastbuild/bin/api/_objs/main/main.pic.o -fPIC -iquote . -iquote bazel-out/k8-fastbuild/bin -iquote external/bazel_tools -iquote bazel-out/k8-fastbuild/bin/external/bazel_tools -std=c++17 -fno-canonical-system-headers -Wno-builtin-macro-redefined -D__DATE__=\"redacted\" -D__TIMESTAMP__=\"redacted\" -D__TIME__=\"redacted\" -c api/main.cc -o bazel-out/k8-fastbuild/bin/api/_objs/main/main.pic.o",
    "file": "api/main.cc",
    "output": "bazel-out/k8-fastbuild/bin/api/_objs/main/main.pic.o"
  }
]

And clang-include-cleaner ran correctly. The file path was api/main.cc, and the compiler invocation’s working directory was /home/leebc/.cache/bazel/_bazel_leebc/5ea2deb937e75f0bf29ba4c49931e67d/execroot/_main, so clang-include-cleaner correctly adjusted the path to /home/leebc/.cache/bazel/_bazel_leebc/5ea2deb937e75f0bf29ba4c49931e67d/execroot/_main/api/main.cc, which was correctly symlinked.

If there are any other cases that I missed, please let me know. I’ll try to reproduce the issue and fix it.

kadircet · 2024-10-09T09:40:00Z

so clang-include-cleaner correctly adjusted the path to /home/leebc/.cache/bazel/_bazel_leebc/5ea2deb937e75f0bf29ba4c49931e67d/execroot/_main/api/main.cc, which was correctly symlinked.

in our case this is symlinked to an immutable copy of the source file (to make sure builds don't act weird if you edit some sources while the build is running). hence it'll be a regression.

What's the concern about not performing this directly as relative to the process' working directory? That approach doesn't rely on any assumptions about the build system, or filesystem layout hence feels much more robust.

Some Bazel systems might have a read-only file system in the build directory. In such cases, the output file should be written to the original source file. Adjusted the logic to find the correct output directory based on the compilation database and the current working directory.

bc-lee · 2024-10-09T10:48:48Z

in our case this is symlinked to an immutable copy of the source file (to make sure builds don't act weird if you edit some sources while the build is running). hence it'll be a regression.

What's the concern about not performing this directly as relative to the process' working directory? That approach doesn't rely on any assumptions about the build system, or filesystem layout hence feels much more robust.

It’s unfortunate that we can't apply unified logic for determining the actual output file. It's especially disappointing given that I’m using GN and you’re using Bazel, both of which are build systems from Google.

I’ve adjusted the logic to find the correct output file path before the compiler invocation starts. Please take another look and let me know if there's anything else I should consider.

kadircet

thanks a lot for the revision!

One concern is, we can't really use getAllCompileCommands() reliably either, not all compilation databases implement that, and even if they do this is gonna be an overkill. For example for chromium we'll iterate over tens of thousands of compile flags, just to modify a handful of files. I've made some suggestions in line.

Also I think it's easier if we put absolute-paths from CDB into the EditedFiles and just remap them to absolute paths per tools working directory afterwards when we can.

kadircet · 2024-10-09T14:12:40Z

clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp

@@ -305,7 +342,32 @@ int main(int argc, const char **argv) {
    }
  }

-  clang::tooling::ClangTool Tool(OptionsParser->getCompilations(),
+  auto &CompilationDatabase = OptionsParser->getCompilations();


i think this can be simplified with something like:

auto VFS = llvm::vfs::getRealFileSystem(); auto &CDB = OptionsParser->getCompilations(); std::map<std::string, std::string> CDBToAbsPaths; for (auto &Source : OptionsParser->getSourcePathList()) { llvm::SmallString<256> AbsPath(Source); if (auto Err = VFS.makeAbsolute(AbsPath)) { llvm::errs() << "Failed to get absolute path for " << Source << " : " << Err.message() << '\n'; return 1; } for(auto Cmd : CDB.getCompilecommands(AbsPath)) { llvm::SmallString<256> CDBPath(Cmd.Filename); llvm::sys::fs::make_absolute(Cmd.Directory, CDBPath); CDBToAbsPaths[CDBPath] = AbsPath; } } .... if (Edit) { for (const auto &NameAndContent : Factory.editedFiles()) { llvm::StringRef FileName = NameAndContent.first(); if (auto It = CDBToAbsPaths.find(FileName); It != CDBToAbsPaths.end()) FileName = It->second; ... } }

I didn't simplify it much, but I tried to explain my intent as clearly as possible. However, I feel the code is somewhat verbose.

kadircet · 2024-10-09T14:22:42Z

clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp

@@ -173,6 +173,7 @@ class Action : public clang::ASTFrontendAction {
    if (!HTMLReportPath.empty())
      writeHTML();

+    // Source File's path relative to compilation database.
    llvm::StringRef Path =


can we keep reporting with abs-paths here, i.e:

llvm::SmallString<256> AbsPath(SM.getFileEntryRefForID(SM.getMainFileID())->getName()); SM.getFileManager().makeAbsolutePath(AbsPath); ...

This way we can recognize these paths in main and perform the mapping accordingly.

- Store the compilation database file as absolute path - Instead of using `llvm::sys::fs::current_path`, use `FileSystem::makeAbsolute` to get the absolute path of various files - Add more comments to explain the intent of the code

… mismatch

github-actions · 2024-10-10T09:40:01Z

✅ With the latest revision this PR passed the C/C++ code formatter.

kadircet · 2024-10-10T10:08:41Z

clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp

+  // CDBToAbsPaths is a map from the path in the compilation database to the
+  // writable absolute path of the file.
+  std::map<std::string, std::string> CDBToAbsPaths;
+  if (Edit) {


nit: I don't think it saves much to do this only when Edit is on, parsing C++ is way slower than anything we can do here over a couple of filenames.

Done. Removed the check for Edit mode.

kadircet · 2024-10-10T10:11:09Z

clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp

+      if (Cmds.empty()) {
+        // Try with the original path.
+        Cmds = CDB.getCompileCommands(Source);
+        if (Cmds.empty()) {
+          continue;
+        }
+      }


this fallback isn't necessary, clang-invocation underneath will also use the AbsPath as-is, and it'll skip the file if it couldn't find any compile flags for it.

I think it's better to just fail early in this case, similar to above.

Done. I checked, and even if we don't specify a compilation database, clang will create one using the FixedCompilationDatabase class. So, I added an error and return early if the compilation database can't be found.

kadircet · 2024-10-10T10:13:02Z

clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp

+      // We only need the first command to get the directory.
+      auto Cmd = Cmds[0];


again, underlying clang-invocation will run the compiler for every compile command we received. hence it isn't enough to just do this for the first command, as each command can have a different (Directory, Filename) combination. also it doesn't hurt even if we get duplicates.

kadircet · 2024-10-10T12:26:02Z

clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp

+      if (llvm::sys::path::is_absolute(CDBPath)) {
+        // If the path in the compilation database is already absolute, we don't
+        // need to do anything.
+        CDBToAbsPaths[static_cast<std::string>(CDBPath)] =
+            static_cast<std::string>(AbsPath);
+      } else {
+        auto Sept = llvm::sys::path::get_separator();
+        // First, try to find the file based on the compilation database.
+        llvm::Twine FilePathTwine = Directory + Sept + CDBPath;
+        llvm::SmallString<256> FilePath;
+        FilePathTwine.toVector(FilePath);
+        // Check if it is writable.
+        if (llvm::sys::fs::access(FilePath, llvm::sys::fs::AccessMode::Write) !=
+            std::error_code()) {
+          // If not, try to find the file based on the current working
+          // directory, as some Bazel invocations may not set the compilation
+          // invocation's filesystem as non-writable. In such cases, we can
+          // find the file based on the current working directory.
+          FilePath = Source;
+          if (auto EC = VFS->makeAbsolute(CDBPath)) {
+            llvm::errs() << "Failed to get absolute path for " << CDBPath
+                         << " : " << EC.message() << '\n';
+            return 1;
+          }
+          if (llvm::sys::fs::access(FilePath,
+                                    llvm::sys::fs::AccessMode::Write) !=
+              std::error_code()) {
+            llvm::errs() << "Failed to find a writable path for " << Source
+                         << '\n';
+            return 1;
+          }
+        }
+        CDBToAbsPaths[static_cast<std::string>(CDBPath)] =
+            static_cast<std::string>(FilePath);
+      }
+    }
+  }


llvm::sys::fs::make_absolute(Cmd.Directory, CDBPath); already performs concatenation only if CDBPath is not an absolute path, you don't re-implement all of this.

kadircet · 2024-10-10T12:26:26Z

clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp

+      if (llvm::sys::path::is_absolute(CDBPath)) {
+        // If the path in the compilation database is already absolute, we don't
+        // need to do anything.
+        CDBToAbsPaths[static_cast<std::string>(CDBPath)] =


we prefer std::string(CDBPath) or CDBPath.str().str() to static_cast. (also for other occurences of static_cast)

kadircet · 2024-10-10T12:29:49Z

clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp

+        if (llvm::sys::fs::access(FilePath, llvm::sys::fs::AccessMode::Write) !=
+            std::error_code()) {


i don't see why we need to complicate the logic by checking for this. is there any reasons to not always map to file-path relative to current process?

i am pretty convinced that the user intention is to almost always edit the file path as referred to during invocation, e.g. cd /foo && clang-include-cleaner path/to/foo.cc is always meant to edit /foo/path/to/foo.cc no matter what file path we get from CDB.

i can see how this might also work, but I prefer to maintain code that has as few special cases as possible. so unless something is indeed breaking with this simplification, can you please get rid of this case?

It turns out we don't need to check for this. I removed those checks.

kadircet · 2024-10-10T12:32:12Z

clang-tools-extra/include-cleaner/test/tool.cpp

@@ -48,3 +48,13 @@ int x = foo();
 //        RUN: clang-include-cleaner -edit --ignore-headers="foobar\.h,foo\.h" %t.cpp -- -I%S/Inputs/ 
 //        RUN: FileCheck --match-full-lines --check-prefix=EDIT2 %s < %t.cpp
 //  EDIT2-NOT: {{^}}#include "foo.h"{{$}}
+
+//        RUN: mkdir -p $(dirname %t)/out


you can use %T instead of dirname %t, you can find full list in https://llvm.org/docs/CommandGuide/lit.html#substitutions, same below.

also it's safer to run a rm -rf %T beforehand to make sure we're starting clean

It turns out %T is also deprecated. I replaced my shell invocation with %t.dir, as this is the recommended method, as described in https://reviews.llvm.org/D69572. I also slightly modified the test invocation so that it works on Windows as well. (Tested on a local Windows machine.)

kadircet · 2024-10-10T12:39:08Z

clang-tools-extra/include-cleaner/test/tool.cpp

@@ -49,12 +49,15 @@ int x = foo();
 //        RUN: FileCheck --match-full-lines --check-prefix=EDIT2 %s < %t.cpp
 //  EDIT2-NOT: {{^}}#include "foo.h"{{$}}

+// This test has commands that rely on shell capabilities that won't execute
+// correctly on Windows e.g. subshell execution
+//   REQUIRES: shell


this doesn't just affect that particular run, affects the whole, can you move the test into its own file ?

This was explained in the previous comment. It now works on Windows as well.

- Removed the check for Edit mode. - Removed unnecessary fallback. - Various cleanups. - Make the test work on Windows as well.

kadircet

thanks for bearing with me, LGTM!

kadircet · 2024-10-14T12:11:31Z

clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp

+  // CDBToAbsPaths is a map from the path in the compilation database to the
+  // writable absolute path of the file.
+  std::map<std::string, std::string> CDBToAbsPaths;
+  // if Edit is enabled, `Factory.editedFiles()` will contain the final code,


can you also drop the mention of Edit from the comment?

kadircet · 2024-10-14T12:15:46Z

clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp

+      // Try with the original path.
+      Cmds = CDB.getCompileCommands(Source);


this fallback is still not needed. if we can't find the flags with absolute path, it's fine to bail out.

kadircet · 2024-10-14T12:18:39Z

clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp

+  for (auto &Source : OptionsParser->getSourcePathList()) {
+    llvm::SmallString<256> AbsPath(Source);
+    if (auto Err = VFS->makeAbsolute(AbsPath)) {
+      llvm::errs() << "Failed to get absolute path for " << Source << " : "
+                   << Err.message() << '\n';
+      return 1;
+    }
+    std::vector<clang::tooling::CompileCommand> Cmds =
+        CDB.getCompileCommands(AbsPath);
+    if (Cmds.empty()) {
+      // Try with the original path.
+      Cmds = CDB.getCompileCommands(Source);
+      if (Cmds.empty()) {
+        // It should be found in the compilation database, even user didn't
+        // specify the compilation database, the `FixedCompilationDatabase` will
+        // create an entry from the arguments. So it is an error if we can't
+        // find the compile commands.
+        llvm::errs() << "No compile commands found for " << Source << '\n';
+        return 1;
+      }
+    }
+    for (const auto &Cmd : Cmds) {
+      llvm::SmallString<256> CDBPath(Cmd.Filename);
+      std::string Directory(Cmd.Directory);
+      llvm::sys::fs::make_absolute(Cmd.Directory, CDBPath);
+      CDBToAbsPaths[std::string(CDBPath)] = std::string(AbsPath);
+    }
+  }


nit: you can extract all of this logic to a helper function:

llvm::Expected<std::map<...>> mapInputsToAbsPaths(...); ... int main(...) { ... auto CDBToAbsPaths = mapInputsToAbsPaths(CDB, Opts->OptionsParser->getSourcePathList()); if (!CDBToAbsPaths) { return 1; } ... }

- Removed unnecessary mention of Edit - Removed fallback logic when the absolute path isn't found - Extracted mapInputsToAbsPaths as a separate function

bc-lee · 2024-10-14T14:46:57Z

Thanks for the review!

kadircet · 2024-10-17T07:59:48Z

btw, LMK if you don't have commit access and I should merge this for you

bc-lee · 2024-10-17T08:04:13Z

I don't have commit access to LLVM. Please merge this PR. Thanks!

bc-lee · 2024-10-17T08:51:15Z

I will fix the following error within an hour (currently away from keyboard).

FAILED: tools/clang/tools/extra/include-cleaner/tool/CMakeFiles/clang-include-cleaner.dir/IncludeCleaner.cpp.o 
ccache /home/docker/llvm-external-buildbots/clang.17.0.6/bin/clang++ --gcc-toolchain=/gcc-toolchain/usr -DGTEST_HAS_RTTI=0 -DLLVM_BUILD_STATIC -D_DEBUG -D_GLIBCXX_ASSERTIONS -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/build/tools/clang/tools/extra/include-cleaner/tool -I/home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/clang-tools-extra/include-cleaner/tool -I/home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/clang/include -I/home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/build/tools/clang/include -I/home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/build/include -I/home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/llvm/include -I/home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/clang-tools-extra/include-cleaner/include -I/home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/clang-tools-extra/include-cleaner/tool/../lib -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -fno-common -Woverloaded-virtual -Wno-nested-anon-types -O3 -DNDEBUG -std=c++17  -fno-exceptions -funwind-tables -fno-rtti -UNDEBUG -MD -MT tools/clang/tools/extra/include-cleaner/tool/CMakeFiles/clang-include-cleaner.dir/IncludeCleaner.cpp.o -MF tools/clang/tools/extra/include-cleaner/tool/CMakeFiles/clang-include-cleaner.dir/IncludeCleaner.cpp.o.d -o tools/clang/tools/extra/include-cleaner/tool/CMakeFiles/clang-include-cleaner.dir/IncludeCleaner.cpp.o -c /home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp
/home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp:302:14: error: moving a temporary object prevents copy elision [-Werror,-Wpessimizing-move]
  302 |       return std::move(llvm::errorCodeToError(Err));
      |              ^
/home/buildbots/llvm-external-buildbots/workers/ppc64le-clang-rhel-test/clang-ppc64le-rhel/llvm-project/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp:302:14: note: remove std::move call here
  302 |       return std::move(llvm::errorCodeToError(Err));
      |              ^~~~~~~~~~                           ~
1 error generated.

…es (llvm#111375) If the current working directory of `clang-include-cleaner` differs from the directory of the input files specified in the compilation database, it doesn't adjust the input file paths properly. As a result, `clang-include-cleaner` either writes files to the wrong directory or fails to write files altogether. This pull request fixes the issue by adjusting the input file paths based on the directory specified in the compilation database. If that directory is not writable, `clang-include-cleaner` will write the output relative to the current working directory. Fixes llvm#110843.

llvmbot added the clang-tools-extra label Oct 7, 2024

bc-lee mentioned this pull request Oct 7, 2024

clang-include-cleaner: Failed to apply edits ... No such file or directory #110843

Closed

kadircet requested changes Oct 8, 2024

View reviewed changes

bc-lee requested a review from kadircet October 9, 2024 10:48

kadircet reviewed Oct 9, 2024

View reviewed changes

bc-lee added 2 commits October 10, 2024 18:02

Apply code review feedback

2409d6f

- Store the compilation database file as absolute path - Instead of using `llvm::sys::fs::current_path`, use `FileSystem::makeAbsolute` to get the absolute path of various files - Add more comments to explain the intent of the code

Add test cases where the cwd and the compilation database's directory…

0c9477d

… mismatch

bc-lee requested a review from kadircet October 10, 2024 09:38

Fix the previous commit that removed existing test cases

dec9be8

bc-lee added 2 commits October 10, 2024 18:43

Make clang-format happy

b39f36e

Fix unittest failures in Windows

3a0f02e

kadircet reviewed Oct 10, 2024

View reviewed changes

Apply review comments.

c65d828

- Removed the check for Edit mode. - Removed unnecessary fallback. - Various cleanups. - Make the test work on Windows as well.

bc-lee requested a review from kadircet October 13, 2024 11:26

kadircet approved these changes Oct 14, 2024

View reviewed changes

Apply review comments

11f78a3

- Removed unnecessary mention of Edit - Removed fallback logic when the absolute path isn't found - Extracted mapInputsToAbsPaths as a separate function

kadircet merged commit 4cda28c into llvm:main Oct 17, 2024
8 checks passed

bc-lee deleted the feature/fix-clang-include-cleaner-path branch October 17, 2024 09:33

bc-lee mentioned this pull request Oct 17, 2024

[clang-include-cleaner] Fix -Wpessimizing-move warning introduced in 4cda28c1ada702a08f6960eb4c93919187c1d4d1 #112684

Closed

		// We only need the first command to get the directory.
		auto Cmd = Cmds[0];

		if (llvm::sys::fs::access(FilePath, llvm::sys::fs::AccessMode::Write) !=
		std::error_code()) {

		// Try with the original path.
		Cmds = CDB.getCompileCommands(Source);

[clang-include-cleaner] Fix incorrect directory issue for writing files #111375

[clang-include-cleaner] Fix incorrect directory issue for writing files #111375

Conversation

bc-lee commented Oct 7, 2024 • edited Loading

llvmbot commented Oct 7, 2024

bc-lee commented Oct 7, 2024

kadircet left a comment

Choose a reason for hiding this comment

bc-lee commented Oct 9, 2024

kadircet commented Oct 9, 2024

bc-lee commented Oct 9, 2024 • edited Loading

kadircet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kadircet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bc-lee commented Oct 14, 2024

kadircet commented Oct 17, 2024

bc-lee commented Oct 17, 2024

bc-lee commented Oct 17, 2024

bc-lee commented Oct 7, 2024 •

edited

Loading

bc-lee commented Oct 9, 2024 •

edited

Loading

github-actions bot commented Oct 10, 2024 •

edited

Loading