[clang-doc] switched from using relative to absolute paths #93281

PeterChou1 · 2024-05-24T09:08:41Z

This patches changes the way clang-doc index navigation works, previously it was based a relative path approach, this approach is error prone and lead to wrong paths for the anchor tag. The new navigation way is based on absolute paths and should work and be less confusing codewise.
Because the differences with serving over a http server and viewing via file system I also added export a RootPath variable to the index_json.js file

llvmbot · 2024-05-24T09:09:11Z

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

Author: None (PeterChou1)

Changes

issue: #92867

I solved the problem by making the js use absolute path instead relative I think this also makes it more permanent since there is no need to compute relative path anymore

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

1 Files Affected:

(modified) clang-tools-extra/clang-doc/assets/index.js (+35-37)

diff --git a/clang-tools-extra/clang-doc/assets/index.js b/clang-tools-extra/clang-doc/assets/index.js
index a5ac90f2e06e7..fe35e706cc98d 100644
--- a/clang-tools-extra/clang-doc/assets/index.js
+++ b/clang-tools-extra/clang-doc/assets/index.js
@@ -1,48 +1,46 @@
-// Append using posix-style a file name or directory to Base
-function append(Base, New) {
-  if (!New)
-    return Base;
-  if (Base)
-    Base += "/";
-  Base += New;
-  return Base;
-}
-
-// Get relative path to access FilePath from CurrentDirectory
-function computeRelativePath(FilePath, CurrentDirectory) {
-  var Path = FilePath;
-  while (Path) {
-    if (CurrentDirectory == Path)
-      return FilePath.substring(Path.length + 1);
-    Path = Path.substring(0, Path.lastIndexOf("/"));
-  }
-
-  var Dir = CurrentDirectory;
-  var Result = "";
-  while (Dir) {
-    if (Dir == FilePath)
-      break;
-    Dir = Dir.substring(0, Dir.lastIndexOf("/"));
-    Result = append(Result, "..")
+function genLink(Ref) {
+  var Path = `${window.location.protocol}//${window.location.host}/${Ref.Path}`;
+  if (Ref.RefType !== "namespace") {
+    if (Ref.Path === "") {
+      Path = `${Path}${Ref.Name}.html`;
+    }
+    else {
+      Path = `${Path}/${Ref.Name}.html`;
+    }
   }
-  Result = append(Result, FilePath.substring(Dir.length))
-  return Result;
-}
-
-function genLink(Ref, CurrentDirectory) {
-  var Path = computeRelativePath(Ref.Path, CurrentDirectory);
-  if (Ref.RefType == "namespace")
-    Path = append(Path, "index.html");
-  else
-    Path = append(Path, Ref.Name + ".html")
 
-    ANode = document.createElement("a");
+  ANode = document.createElement("a");
   ANode.setAttribute("href", Path);
   var TextNode = document.createTextNode(Ref.Name);
   ANode.appendChild(TextNode);
   return ANode;
 }
 
+function genHTMLOfIndex(Index, CurrentDirectory, IsOutermostList) {
+  // Out will store the HTML elements that Index requires to be generated
+  var Out = [];
+  if (Index.Name) {
+    var SpanNode = document.createElement("span");
+    var TextNode = document.createTextNode(Index.Name);
+    SpanNode.appendChild(genLink(Index));
+    Out.push(SpanNode);
+  }
+  if (Index.Children.length == 0)
+    return Out;
+  // Only the outermost list should use ol, the others should use ul
+  var ListNodeName = IsOutermostList ? "ol" : "ul";
+  var ListNode = document.createElement(ListNodeName);
+  for (Child of Index.Children) {
+    var LiNode = document.createElement("li");
+    ChildNodes = genHTMLOfIndex(Child, CurrentDirectory, false);
+    for (Node of ChildNodes)
+      LiNode.appendChild(Node);
+    ListNode.appendChild(LiNode);
+  }
+  Out.push(ListNode);
+  return Out;
+}
+
 function genHTMLOfIndex(Index, CurrentDirectory, IsOutermostList) {
   // Out will store the HTML elements that Index requires to be generated
   var Out = [];

ilovepi

I'm not completely sure we want to use absolute paths, but since this mostly seems related to URLs and not file system paths, its probably fine.

Please add some tests for this. It's preferable if we can pre-commit tests with the existing behavior, and then can easily see how this patch modifies the old/undesirable behavior.

Lastly, please update the title:

and update the description to describe what this fixes and how, and links to the GitHub issue using fixes or something equivalent from https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue

clang-tools-extra/clang-doc/assets/index.js

github-actions · 2024-06-29T08:30:59Z

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

ilovepi

This is almost there. I've left a few minor comments, but I'd like to see better testing here. I'm awfully surprised we don't need to update any test lines, other than the 1 JS check. It would really help to see how the output is changing, if that was captured in the tests.

ilovepi · 2024-06-29T16:15:00Z

clang-tools-extra/test/clang-doc/basic-project.test

@@ -7,6 +7,7 @@
 // RUN: FileCheck %s -input-file=%t/docs/GlobalNamespace/Rectangle.html -check-prefix=HTML-RECTANGLE
 // RUN: FileCheck %s -input-file=%t/docs/GlobalNamespace/Circle.html -check-prefix=HTML-CIRCLE

+// JSON-INDEX: var RootPath = "{{.*}}";


This is a URL, right? if so, is there a reason it wouldn't match, regardless of platform? I'm a bit surprised this is the only change required to the tests, so I'm wondering if we're doing a good job checking things in our existing HTML tests.

This isn't a url, this variable is a path to the directory where clang-doc is built from. I wanted the html to still render properly when viewed via the local filesystem so I added this export variable.
Most the changes that is reflected in this patch can only be seen once the browser loads the page and javascript generates the sidebar with the indexes, I don't think its possible to see the changes reflected unless we incorporate some sort javascript test framework. Another approach could be to just generate the indexes via the c++ html generator than we be able to see the changes.
I've added some test that test the different behaviour when we generate clang-doc via a relative path or an absolute one.

Oh, then I'm not 100% sure we want to replace all the \ w/ /. I know you mentioned browser behavior, but I'm not 100% certain that's a great motivation. When I thought it was a URL, there's obviously no issue, but, for a file system path, I'm hesitant to say with confidence that there's no downside.

Also, given the test updates, IDK if this check is particularly useful. I don't see much difference in the check as is, and just

// JSON-INDEX: var RootPath =

clang-tools-extra/clang-doc/HTMLGenerator.cpp

clang-tools-extra/clang-doc/assets/index.js

ilovepi · 2024-06-30T15:14:59Z

clang-tools-extra/test/clang-doc/test-path-abs.cpp

+// RUN: rm -rf %t
+// RUN: mkdir %t


nit: Can be one line w/ &&

clang-tools-extra/test/clang-doc/test-path-abs.cpp

clang-tools-extra/test/clang-doc/test-path-relative.cpp

ilovepi · 2024-06-30T15:41:23Z

clang-tools-extra/test/clang-doc/test-path-abs.cpp

+// RUN: echo "CHECK: var RootPath = \"%/t/docs\";" > %t/check.txt
+// RUN: cp "%s" "%t/test.cpp"


I see you're trying to work around the path inconsistency issues, but that's just an end-around our testing practices, and we shouldn't be doing it. Tests server as documentation, and generating a test w/ CHECK lines undermines that pretty significantly. IMO if the test isn't useful w/ the regular expression, it isn't useful. As it stands I also can't reason about how the behavior here is changing as a result of this patch, so I don' think this is a particularly good/useful check.

If possible, it would be good to add these tests in another patch(e.g. pre-commit them testing the current behavior), and then update these to make them pass. That requires either stacking the PR using spr, or just managing the github PRs carefully yourself. Either is fine.

ilovepi · 2024-06-30T15:43:51Z

clang-tools-extra/test/clang-doc/test-path-abs.cpp

+// RUN: mkdir %t
+// RUN: echo "CHECK: var RootPath = \"%/t/docs\";" > %t/check.txt
+// RUN: cp "%s" "%t/test.cpp"
+// RUN: clang-doc --format=html --executor=standalone -p %t %t/test.cpp --output=%t/docs


Suggested change

// RUN: clang-doc --format=html --executor=standalone -p %t %t/test.cpp --output=%t/docs

// RUN: clang-doc --format=html --executor=standalone %s --output=%t

I don't think you need -p, and I don't see a good reason why you need a subdirectory for docs.

ilovepi · 2024-06-30T15:44:41Z

clang-tools-extra/test/clang-doc/test-path-abs.cpp

+// RUN: echo "CHECK: var RootPath = \"%/t/docs\";" > %t/check.txt
+// RUN: cp "%s" "%t/test.cpp"
+// RUN: clang-doc --format=html --executor=standalone -p %t %t/test.cpp --output=%t/docs
+// RUN: FileCheck %t/check.txt -input-file=%t/docs/index_json.js


Suggested change

// RUN: FileCheck %t/check.txt -input-file=%t/docs/index_json.js

// RUN: FileCheck %s -input-file=%t/index_json.js --check-prefix=SOME_PREFIX

ilovepi · 2024-07-10T01:22:34Z

any progress here?

PeterChou1 · 2024-07-12T11:23:27Z

any progress here?

I wanted to merge the test cases prs so we could view the regressions from the tests

ilovepi

LGTM, but can we improve the check for RootPath somehow? I feel like there should be something we can match there.

ilovepi · 2024-07-24T23:30:34Z

clang-tools-extra/test/clang-doc/test-path-abs.cpp

+// RUN: FileCheck %s -input-file=%t/index_json.js  -check-prefix=JSON-INDEX
+// RUN: rm -rf %t
+
+// JSON-INDEX: var RootPath = "{{.*}}";


Is there a way to make this check more meaningful? Can we match nothing in the path itself?

ilovepi · 2024-07-24T23:37:07Z

Hmm, seems like tests are failing. PTAL.

ilovepi · 2024-07-24T23:59:17Z

clang-tools-extra/test/clang-doc/test-path-abs.cpp

@@ -0,0 +1,7 @@
+// RUN: rm -rf %t && mkdir %t
+// RUN: cp "%s" "%t/test.cpp"


Looks like I missed this. Please don’t copy the test file around. To be blunt this pattern has been brought up in multiple PRs and even in previous incarnations of this one. Use the same conventions we’ve insisted on in other tests here.

Sorry, the reason why I kept using this pattern is because I copied the same test setups in the original test for clang-doc here and here

Summary: fixes #92867 This patches changes the way clang-doc index navigation works, previously it was based a relative path approach, this approach is error prone and lead to wrong paths for the anchor tag. The new navigation way is based on absolute paths and should work and be less confusing codewise. Because the differences with serving over a http server and viewing via file system I also added export a RootPath variable to the index_json.js file Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60250723

llvmbot added the clang-tools-extra label May 24, 2024

ilovepi requested changes May 24, 2024

View reviewed changes

clang-tools-extra/clang-doc/assets/index.js Outdated Show resolved Hide resolved

clang-tools-extra/clang-doc/assets/index.js Outdated Show resolved Hide resolved

ilovepi added the clang-doc label May 24, 2024

ilovepi requested a review from petrhosek May 24, 2024 17:26

PeterChou1 changed the title ~~clang-doc switched from using relative to absolute paths~~ [clang-doc] switched from using relative to absolute paths May 24, 2024

PeterChou1 force-pushed the clang-doc-abs-path branch from 48ceff1 to be0fb1a Compare June 29, 2024 09:16

ilovepi reviewed Jun 29, 2024

View reviewed changes

ilovepi reviewed Jun 30, 2024

View reviewed changes

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

ilovepi reviewed Jun 30, 2024

View reviewed changes

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

ilovepi reviewed Jun 30, 2024

View reviewed changes

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

ilovepi reviewed Jun 30, 2024

View reviewed changes

PeterChou1 added 10 commits July 24, 2024 18:42

clang-doc switched from using relative to absolute paths

f3eec97

remove duplicate function

4d4197f

[clang-doc] modify paths for anchors in js file

61e9da0

[clang-doc] add test for asset paths

fd3ff81

[clang-doc] fix tests

f786396

[clang-doc] fix missing - in output

40a52e5

[clang-doc] fix platform

6b47fee

[clang-doc] paths

ca1fb33

[clang-doc] pr comments

3142445

[clang-doc] address pr comments

02a1c5a

PeterChou1 force-pushed the clang-doc-abs-path branch from da5b1c1 to 02a1c5a Compare July 24, 2024 22:48

[clang-doc] rework tests

1732676

ilovepi approved these changes Jul 24, 2024

View reviewed changes

[clang-doc] fix broken tests

c2d9e15

ilovepi reviewed Jul 24, 2024

View reviewed changes

[clang-doc] fix clang-doc error

9f02856

PeterChou1 merged commit 91450f1 into llvm:main Jul 25, 2024
7 checks passed

		// RUN: echo "CHECK: var RootPath = \"%/t/docs\";" > %t/check.txt
		// RUN: cp "%s" "%t/test.cpp"

	// RUN: clang-doc --format=html --executor=standalone -p %t %t/test.cpp --output=%t/docs
	// RUN: clang-doc --format=html --executor=standalone %s --output=%t

	// RUN: FileCheck %t/check.txt -input-file=%t/docs/index_json.js
	// RUN: FileCheck %s -input-file=%t/index_json.js --check-prefix=SOME_PREFIX

		@@ -0,0 +1,7 @@
		// RUN: rm -rf %t && mkdir %t
		// RUN: cp "%s" "%t/test.cpp"

[clang-doc] switched from using relative to absolute paths #93281

[clang-doc] switched from using relative to absolute paths #93281

Uh oh!

Conversation

PeterChou1 commented May 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented May 24, 2024

Uh oh!

ilovepi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Jun 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ilovepi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PeterChou1 Jun 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ilovepi commented Jul 10, 2024

Uh oh!

PeterChou1 commented Jul 12, 2024

Uh oh!

ilovepi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ilovepi commented Jul 24, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

PeterChou1 commented May 24, 2024 •

edited

Loading

github-actions bot commented Jun 29, 2024 •

edited

Loading

PeterChou1 Jun 30, 2024 •

edited

Loading