Fix Default Asset File locator for clang docs #97505
Conversation
|
@llvm/pr-subscribers-clang-tools-extra Author: Joshua Batista (bob80905) ChangesIn The locator assumed that the executable that is running was in the first example above, and in the case that an executable under Debug / Release was run, the index.js file wouldn't be found. Full diff: https://github.com/llvm/llvm-project/pull/97505.diff 1 Files Affected:
diff --git a/clang-tools-extra/clang-doc/tool/ClangDocMain.cpp b/clang-tools-extra/clang-doc/tool/ClangDocMain.cpp
index 6198a6e0cdcc3..b97fa715f9e67 100644
--- a/clang-tools-extra/clang-doc/tool/ClangDocMain.cpp
+++ b/clang-tools-extra/clang-doc/tool/ClangDocMain.cpp
@@ -167,7 +167,13 @@ llvm::Error getDefaultAssetFiles(const char *Argv0,
llvm::SmallString<128> AssetsPath;
AssetsPath = llvm::sys::path::parent_path(NativeClangDocPath);
- llvm::sys::path::append(AssetsPath, "..", "share", "clang-doc");
+ llvm::sys::path::append(AssetsPath, "..");
+ llvm::SmallString<128> tempCopyDbg = AssetsPath;
+ llvm::sys::path::append(tempCopyDbg, "Debug");
+ // index.js may be in the debug
+ if (!llvm::sys::fs::is_directory(tempCopyDbg))
+ llvm::sys::path::append(AssetsPath, "..");
+ llvm::sys::path::append(AssetsPath, "share", "clang-doc");
llvm::SmallString<128> DefaultStylesheet;
llvm::sys::path::native(AssetsPath, DefaultStylesheet);
llvm::sys::path::append(DefaultStylesheet,
|
|
An important assumption I'm making here is that the Debug directory cannot exist without the Release directory. If this assumption doesn't hold, then this fix will fail if only the release directory exists and debug is missing. I believe by the nature of how the llvm project is generated, both directories are generated at all times. |
There was a problem hiding this comment.
I don't think this is the right fix. The Release/Debug directories only exist in the build tree, so this is incurring an unnecessary cost in an installed clang-doc and introducing a potentially very strange behaviour for weird install locations.
Probably the simplest approach to fixing the problem would be to duplicate the share/clang-doc directory per config. This is a bit annoying because of the redundancy, but it's probably better than pursuing something more elaborate.
|
I don't think that the current description matches the change? Does this mean that there'll likely be 3 copies of each file. One per configuration, and another copy just in the binary directory? In a multi-config build do we still need the copy in the binary directory? |
|
Updated the description. Yes, one per configuration, and one underneath the binary directory. |
We shouldn't need the one in the binary directory - IIUC one per configuration will be sufficient, and the one that's never used is potentially confusing. Please also test locally that the install target does the right thing with these changes. You can use |
There was a problem hiding this comment.
It's kind of unfortunate how this works dependency-wise - we end up creating "Debug/share/clang-doc/${f}" even when we only build "Release" and vice-versa, as the "copy-clang-doc-assets" explicitly depends on each version of this file. Also the actual install copies the file directly from the source directory, so if we did end up actually configuring these files per config for some reason there's some potential for confusion.
However, the cmake-fu to do this any better is beyond me, and the current state is that we actually have broken bots because of this, so I think the solution here is Good Enough to get in and get the bots unblocked.
| set(out_files) | ||
|
|
||
| function(copy_files_to_dst src_dir dst_dir file) | ||
| set(src "${src_dir}/${file}") | ||
| set(dst "${dst_dir}/${file}") | ||
| if (NOT EXISTS ${dst_dir}) | ||
| file(MAKE_DIRECTORY ${dst_dir}) | ||
| endif() |
There was a problem hiding this comment.
I don't believe this is necessary - copy_if_different will do the right thing.
|
Closing this PR since #98099 resolves the issue and has already been merged. |
In
clang-tools-extra\clang-doc\tool\ClangDocMain.cpp, there is a function,getDefaultAssetFilesthat tries to find the location of<build dir>\share\clang-doc\index.js, but fails. This is because there are alternate configurations in setting up the llvm project, and executables in different locations may be run. In some cases, executables are built and run in this location:<build dir>\bin\<executables>and in other cases, this location:
<build dir>\[Debug | Release]\bin\<executables>The locator assumed that the executable that is running was in the first example above, and only ascended one directory, and in the case that an executable under Debug / Release was run, the index.js file wouldn't be found.
This PR changes the way in which the asset files are copied over to the build output directory, by placing the directory containing the asset files directly under whichever specific build-output configuration directory is being processed by cmake.
In this way, regardless of which configuration's executable runs
getDefaultAssetFiles, the asset files will be found directly underneath the current build directory.Fixes #98056