reapply [clang-doc] Add --asset option to clang-doc #96358

PeterChou1 · 2024-06-21T21:00:06Z

reapply #94717
Adds a new option --asset which allows users to specified the asset folder for the html output of clang-doc
related to: #93928

This patch adds a better test for --asset option + fixes bug where clang-doc assumes that user supplied js file is assume to be index.js

)" This reverts commit bf824d9.

llvmbot · 2024-06-21T21:00:38Z

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

Author: None (PeterChou1)

Changes

@ilovepi

re-reverts clang-doc

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

2 Files Affected:

(modified) clang-tools-extra/clang-doc/tool/ClangDocMain.cpp (+81-19)
(added) clang-tools-extra/test/clang-doc/single-source-html.cpp (+2)

diff --git a/clang-tools-extra/clang-doc/tool/ClangDocMain.cpp b/clang-tools-extra/clang-doc/tool/ClangDocMain.cpp
index 5517522d7967d..5a43c70a5ebc3 100644
--- a/clang-tools-extra/clang-doc/tool/ClangDocMain.cpp
+++ b/clang-tools-extra/clang-doc/tool/ClangDocMain.cpp
@@ -81,6 +81,12 @@ static llvm::cl::list<std::string> UserStylesheets(
     llvm::cl::desc("CSS stylesheets to extend the default styles."),
     llvm::cl::cat(ClangDocCategory));
 
+static llvm::cl::opt<std::string> UserAssetPath(
+    "asset",
+    llvm::cl::desc("User supplied asset path to "
+                   "override the default css and js files for html output"),
+    llvm::cl::cat(ClangDocCategory));
+
 static llvm::cl::opt<std::string> SourceRoot("source-root", llvm::cl::desc(R"(
 Directory where processed files are stored.
 Links to definition locations will only be
@@ -127,16 +133,86 @@ std::string getFormatString() {
 // GetMainExecutable (since some platforms don't support taking the
 // address of main, and some platforms can't implement GetMainExecutable
 // without being given the address of a function in the main executable).
-std::string GetExecutablePath(const char *Argv0, void *MainAddr) {
+std::string getExecutablePath(const char *Argv0, void *MainAddr) {
   return llvm::sys::fs::getMainExecutable(Argv0, MainAddr);
 }
 
+llvm::Error getAssetFiles(clang::doc::ClangDocContext &CDCtx) {
+  using DirIt = llvm::sys::fs::directory_iterator;
+  std::error_code FileErr;
+  llvm::SmallString<128> FilePath(UserAssetPath);
+  for (DirIt DirStart = DirIt(UserAssetPath, FileErr),
+                   DirEnd;
+       !FileErr && DirStart != DirEnd; DirStart.increment(FileErr)) {
+    FilePath = DirStart->path();
+    if (llvm::sys::fs::is_regular_file(FilePath)) {
+      if (llvm::sys::path::extension(FilePath) == ".css")
+        CDCtx.UserStylesheets.insert(CDCtx.UserStylesheets.begin(),
+                                     std::string(FilePath));
+      else if (llvm::sys::path::extension(FilePath) == ".js")
+        CDCtx.FilesToCopy.emplace_back(FilePath.str());
+    }
+  }
+  if (FileErr)
+    return llvm::createFileError(FilePath, FileErr);
+  return llvm::Error::success();
+}
+
+llvm::Error getDefaultAssetFiles(const char *Argv0,
+                                 clang::doc::ClangDocContext &CDCtx) {
+  void *MainAddr = (void *)(intptr_t)getExecutablePath;
+  std::string ClangDocPath = getExecutablePath(Argv0, MainAddr);
+  llvm::SmallString<128> NativeClangDocPath;
+  llvm::sys::path::native(ClangDocPath, NativeClangDocPath);
+
+  llvm::SmallString<128> AssetsPath;
+  AssetsPath = llvm::sys::path::parent_path(NativeClangDocPath);
+  llvm::sys::path::append(AssetsPath, "..", "share", "clang");
+  llvm::SmallString<128> DefaultStylesheet;
+  llvm::sys::path::native(AssetsPath, DefaultStylesheet);
+  llvm::sys::path::append(DefaultStylesheet,
+                          "clang-doc-default-stylesheet.css");
+  llvm::SmallString<128> IndexJS;
+  llvm::sys::path::native(AssetsPath, IndexJS);
+  llvm::sys::path::append(IndexJS, "index.js");
+
+  llvm::outs() << "Using default asset: " << AssetsPath << "\n";
+
+  if (!llvm::sys::fs::is_regular_file(IndexJS))
+    return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                   "default index.js file missing at " +
+                                       IndexJS + "\n");
+
+  if (!llvm::sys::fs::is_regular_file(DefaultStylesheet))
+    return llvm::createStringError(
+        llvm::inconvertibleErrorCode(),
+        "default clang-doc-default-stylesheet.css file missing at " +
+            DefaultStylesheet + "\n");
+
+  CDCtx.UserStylesheets.insert(CDCtx.UserStylesheets.begin(),
+                               std::string(DefaultStylesheet));
+  CDCtx.FilesToCopy.emplace_back(IndexJS.str());
+
+  return llvm::Error::success();
+}
+
+llvm::Error getHtmlAssetFiles(const char *Argv0,
+                              clang::doc::ClangDocContext &CDCtx) {
+  if (!UserAssetPath.empty() &&
+      !llvm::sys::fs::is_directory(std::string(UserAssetPath)))
+    llvm::outs() << "Asset path supply is not a directory: " << UserAssetPath
+                 << " falling back to default\n";
+  if (llvm::sys::fs::is_directory(std::string(UserAssetPath)))
+    return getAssetFiles(CDCtx);
+  return getDefaultAssetFiles(Argv0, CDCtx);
+}
+
 int main(int argc, const char **argv) {
   llvm::sys::PrintStackTraceOnErrorSignal(argv[0]);
   std::error_code OK;
 
   const char *Overview =
-    R"(Generates documentation from source code and comments.
+      R"(Generates documentation from source code and comments.
 
 Example usage for files without flags (default):
 
@@ -182,23 +258,9 @@ Example usage for a project using a compile commands database:
       {"index.js", "index_json.js"}};
 
   if (Format == "html") {
-    void *MainAddr = (void *)(intptr_t)GetExecutablePath;
-    std::string ClangDocPath = GetExecutablePath(argv[0], MainAddr);
-    llvm::SmallString<128> NativeClangDocPath;
-    llvm::sys::path::native(ClangDocPath, NativeClangDocPath);
-    llvm::SmallString<128> AssetsPath;
-    AssetsPath = llvm::sys::path::parent_path(NativeClangDocPath);
-    llvm::sys::path::append(AssetsPath, "..", "share", "clang");
-    llvm::SmallString<128> DefaultStylesheet;
-    llvm::sys::path::native(AssetsPath, DefaultStylesheet);
-    llvm::sys::path::append(DefaultStylesheet,
-                            "clang-doc-default-stylesheet.css");
-    llvm::SmallString<128> IndexJS;
-    llvm::sys::path::native(AssetsPath, IndexJS);
-    llvm::sys::path::append(IndexJS, "index.js");
-    CDCtx.UserStylesheets.insert(CDCtx.UserStylesheets.begin(),
-                                 std::string(DefaultStylesheet));
-    CDCtx.FilesToCopy.emplace_back(IndexJS.str());
+    if (auto Err = getHtmlAssetFiles(argv[0], CDCtx)) {
+      llvm::outs() << "warning: " <<  toString(std::move(Err)) << "\n";
+    }
   }
 
   // Mapping phase
diff --git a/clang-tools-extra/test/clang-doc/single-source-html.cpp b/clang-tools-extra/test/clang-doc/single-source-html.cpp
new file mode 100644
index 0000000000000..32f232b9c45a0
--- /dev/null
+++ b/clang-tools-extra/test/clang-doc/single-source-html.cpp
@@ -0,0 +1,2 @@
+// RUN: clang-doc --format=html --executor=standalone %s -output=%t/docs | FileCheck %s
+// CHECK: Using default asset: {{.*}}{{[\/]}}share{{[\/]}}clang
\ No newline at end of file

PeterChou1 · 2024-06-21T21:11:22Z

@ilovepi

ilovepi

LGTM, but wait for Nico to chime in please.

nico · 2024-06-22T20:02:23Z

Sorry, I'm confused, how is this build-system dependent? The patch doesn't change any cmake files, and it changes the behavior of an existing test.

I'm not saying it can't be build-system dependent, but it isn't clear to me why it is. Do you have a few more words on what's happening?

nico · 2024-06-22T20:22:59Z

Ah, I see now, the failing test is also very new (70ec841), and it depends on ade28a7 which the GN build doesn't yet have. But my bot points at this change here for breaking that fairly new end-to-end test. I think it's probably because absence of the css file wasn't an error before, but now it is. It seems fine to me to make that an error, but changing that seems somewhat tangential to what this PR does. Maybe it deserves at least its own "Additionally, make absence of the css file in the asset directory an error because $reason" sentence in the commit message?

nico · 2024-06-22T20:39:15Z

clang-tools-extra/clang-doc/tool/ClangDocMain.cpp

-    CDCtx.UserStylesheets.insert(CDCtx.UserStylesheets.begin(),
-                                 std::string(DefaultStylesheet));
-    CDCtx.FilesToCopy.emplace_back(IndexJS.str());
+    if (auto Err = getHtmlAssetFiles(argv[0], CDCtx)) {


After I've read this in some detail, I'm confused by this patch.

As far as I can tell, getAssetFiles() looks for all .css and .js files in the passed-in directory and appends them. Line 182 still wants the main js file to be called index.js, even if --asset is used (…right?), but only getDefaultAssetFiles() checks for the existence of that file.

The way I would've expected this to work is that --asset just overrides the value of AssetsPath. Is there more discussion on why this doesn't implemented that way somewhere that I missed?

Yes this is an oversight on my part the clang-doc still expect users to provide a file called index.js even if we use the --asset option I've modify the pr to make it so that clang-doc uses whatever js file the user supplies

clang-tools-extra/clang-doc/tool/ClangDocMain.cpp

nico · 2024-06-22T20:42:30Z

clang-tools-extra/test/clang-doc/single-source-html.cpp

@@ -0,0 +1,2 @@
+// RUN: clang-doc --format=html --executor=standalone %s -output=%t/docs | FileCheck %s
+// CHECK: Using default asset: {{.*}}{{[\/]}}share{{[\/]}}clang


A test that only tests for logspam doesn't seem very useful. I would've expected a change adding an --asset option to add a test that uses that option.

I added a much more comprehensive test now I think

nico · 2024-06-22T20:49:46Z

clang-tools-extra/clang-doc/tool/ClangDocMain.cpp

-                                 std::string(DefaultStylesheet));
-    CDCtx.FilesToCopy.emplace_back(IndexJS.str());
+    if (auto Err = getHtmlAssetFiles(argv[0], CDCtx)) {
+      llvm::outs() << "warning: " <<  toString(std::move(Err)) << "\n";


Since errors lead to early returns (and hence behavior differences), they should probably cause an actual error return code instead of just a warning?

nico · 2024-06-22T21:00:46Z

clang-tools-extra/clang-doc/tool/ClangDocMain.cpp

+
+  llvm::SmallString<128> AssetsPath;
+  AssetsPath = llvm::sys::path::parent_path(NativeClangDocPath);
+  llvm::sys::path::append(AssetsPath, "..", "share", "clang");


If we're touching this area anyway: Does it make sense to put clang-doc assets into .../share/clang-doc instead of in .../share/clang?

I agree, we should use share/clang-doc rather than share/clang.

I've modify the pr to change where clang-doc keeps the install file

nico · 2024-06-22T21:06:39Z

(I now ported ade28a7 to the GN build in 3ba7599. Again, apologies for missing this – the GN build shouldn't affect other people. But I'm kind of glad it did since I found several GN-unrelated issues both here and on #93928. Feel free to do with them what you will – except for the stdexcept include over in #93928, please remove that.)

PeterChou1 · 2024-06-24T19:54:57Z

@ilovepi

I've refactor the PR to address Nico comments I think you might want to relook at the PR just in case

ilovepi · 2024-06-24T19:50:31Z

clang-tools-extra/clang-doc/tool/CMakeLists.txt

@@ -25,7 +25,7 @@ set(assets
 )

 set(asset_dir "${CMAKE_CURRENT_SOURCE_DIR}/../assets")
-set(resource_dir "${CMAKE_BINARY_DIR}/share/clang")


Can we do this updating in a standalone patch? Same for the other places where you've updated the path.

Oh, it also fails to update the install target paths, which will be installed in share/clang. So even if we leave that in this patch, you'd have more to update.

this PR is moved to #96555

clang-tools-extra/test/clang-doc/assets.cpp

clang-tools-extra/test/clang-doc/Inputs/basic-project/src/Calculator.cpp

ilovepi · 2024-06-25T00:22:43Z

Can you update the commit message to describe why this can be relanded, and what this patch does? A good rule of thumb is to include the entire original commit message, followed by a short paragraph describing what's changed.

PeterChou1 · 2024-06-25T01:07:30Z

Can you update the commit message to describe why this can be relanded, and what this patch does? A good rule of thumb is to include the entire original commit message, followed by a short paragraph describing what's changed.

Done

ilovepi · 2024-06-25T16:43:31Z

The two complementary patches, making the test free standing, and fixing the default asset path to be share/clang-doc have landed now. Can you update your patch, and resolve conflicts. If that passes CI, then I think we're probably about ready to reland.

ilovepi · 2024-06-25T18:04:17Z

Seems like a tidy test is failing on windows ... can you try updating the branch once more?

PeterChou1 · 2024-06-26T20:04:19Z

Seems like a tidy test is failing on windows ... can you try updating the branch once more?

I think its fixed now?

dyung · 2024-07-03T03:01:41Z

@PeterChou1, your change is causing a test failure when using a non-ninja generator to build because the path name is not as expected. I have put details in #97507, can you take a look?

Reapply llvm#94717 Adds a new option --asset which allows users to specified the asset folder for the html output of clang-doc. This patch adds a better test for --asset option + fixes bug where clang-doc assumes that user supplied js file is assume to be index.js

Revert "Revert "[clang-doc] Add --asset option to clang-doc" (llvm#96354

bb407e7

)" This reverts commit bf824d9.

llvmbot added the clang-tools-extra label Jun 21, 2024

ilovepi self-requested a review June 21, 2024 22:32

PeterChou1 changed the title ~~Revert "Revert "[clang-doc] Add --asset option to clang-doc" (#96354)"~~ reapply [clang-doc] Add --asset option to clang-doc" (#96354)" Jun 21, 2024

ilovepi requested review from nico and petrhosek June 21, 2024 22:33

ilovepi reviewed Jun 21, 2024

View reviewed changes

PeterChou1 changed the title ~~reapply [clang-doc] Add --asset option to clang-doc" (#96354)"~~ reapply [clang-doc] Add --asset option to clang-doc Jun 22, 2024

nico reviewed Jun 22, 2024

View reviewed changes

clang-tools-extra/clang-doc/tool/ClangDocMain.cpp Outdated Show resolved Hide resolved

nico reviewed Jun 22, 2024

View reviewed changes

ilovepi reviewed Jun 24, 2024

View reviewed changes

PeterChou1 force-pushed the re-revert-add-asset branch 2 times, most recently from 253eb8b to ce8f888 Compare June 24, 2024 21:13

ilovepi reviewed Jun 24, 2024

View reviewed changes

clang-tools-extra/test/clang-doc/assets.cpp Outdated Show resolved Hide resolved

clang-tools-extra/test/clang-doc/Inputs/basic-project/src/Calculator.cpp Outdated Show resolved Hide resolved

[clang-doc] address pr comments

c7490ab

PeterChou1 force-pushed the re-revert-add-asset branch from 4494fa6 to c7490ab Compare June 24, 2024 22:04

PeterChou1 added 2 commits June 24, 2024 18:47

[clang-doc] switch loading

48b3dbf

[clang-doc] remove extra --output

6321616

[clang-doc] fix conflict

900869e

[clang-doc] fix default asset path

ada6489

Merge branch 'main' into re-revert-add-asset

2e6ee4c

PeterChou1 requested a review from ilovepi June 27, 2024 19:38

ilovepi merged commit f14ad74 into llvm:main Jun 27, 2024
7 checks passed

dyung mentioned this pull request Jul 3, 2024

Test clang-tools-extra/test/clang-doc/basic-project.test failing when built using non-ninja generator #97507

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

reapply [clang-doc] Add --asset option to clang-doc #96358

reapply [clang-doc] Add --asset option to clang-doc #96358

PeterChou1 commented Jun 21, 2024 •

edited

Loading

llvmbot commented Jun 21, 2024

PeterChou1 commented Jun 21, 2024

ilovepi left a comment

nico commented Jun 22, 2024

nico commented Jun 22, 2024

nico Jun 22, 2024

PeterChou1 Jun 24, 2024

nico Jun 22, 2024

PeterChou1 Jun 24, 2024

nico Jun 22, 2024

nico Jun 22, 2024

petrhosek Jun 23, 2024

PeterChou1 Jun 24, 2024

nico commented Jun 22, 2024 •

edited

Loading

PeterChou1 commented Jun 24, 2024

ilovepi Jun 24, 2024

ilovepi Jun 24, 2024

PeterChou1 Jun 24, 2024

ilovepi commented Jun 25, 2024

PeterChou1 commented Jun 25, 2024

ilovepi commented Jun 25, 2024

ilovepi commented Jun 25, 2024

PeterChou1 commented Jun 26, 2024

dyung commented Jul 3, 2024

		@@ -0,0 +1,2 @@
		// RUN: clang-doc --format=html --executor=standalone %s -output=%t/docs \| FileCheck %s
		// CHECK: Using default asset: {{.*}}{{[\/]}}share{{[\/]}}clang

reapply [clang-doc] Add --asset option to clang-doc #96358

reapply [clang-doc] Add --asset option to clang-doc #96358

Conversation

PeterChou1 commented Jun 21, 2024 • edited Loading

llvmbot commented Jun 21, 2024

PeterChou1 commented Jun 21, 2024

ilovepi left a comment

Choose a reason for hiding this comment

nico commented Jun 22, 2024

nico commented Jun 22, 2024

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

nico commented Jun 22, 2024 • edited Loading

PeterChou1 commented Jun 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilovepi commented Jun 25, 2024

PeterChou1 commented Jun 25, 2024

ilovepi commented Jun 25, 2024

ilovepi commented Jun 25, 2024

PeterChou1 commented Jun 26, 2024

dyung commented Jul 3, 2024

PeterChou1 commented Jun 21, 2024 •

edited

Loading

nico commented Jun 22, 2024 •

edited

Loading