This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Change the url for clang-tidy check documentation
ClosedPublic

Authored by njames93 on Jun 22 2022, 1:50 PM.

Details

Summary

In https://github.com/llvm/llvm-project/commit/6e566bc5523f743bc34a7e26f050f1f2b4d699a8, The directory structure of the documentation for clang-tidy checks was changed, however clangd wasn't updated.
Now all the links generated will point to old dead pages.
This updated clangd to use the new page structure.

Diff Detail

Event Timeline

njames93 created this revision.Jun 22 2022, 1:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 22 2022, 1:50 PM
njames93 requested review of this revision.Jun 22 2022, 1:50 PM
sammccall accepted this revision.Jun 23 2022, 6:56 AM

Thanks!

clang-tools-extra/clangd/Diagnostics.cpp
930

I don't think we should assert on this, how clang-tidy checks are named isn't something clangd can reasonably control.

Instead I'd suggest only returning a value if the condition is met.

This revision is now accepted and ready to land.Jun 23 2022, 6:56 AM
njames93 added inline comments.Jun 23 2022, 10:58 AM
clang-tools-extra/clangd/Diagnostics.cpp
930

The logic behind the assert was that it wouldn't cause any issues on release, but if someone added a check not to the convention it would break debug builds.
But it's not an issue that's expected anyway so can be safely removed.

luckily this document links were introduced only recently, hence they didn't make it to clangd-14, but they'll start being around from clangd-15 and such changes will be breaking all the links in existing clangd's.
i wonder if we should actually point these at releases.llvm.org/CLANG_VERSION/... instead of top of the head:

  • downside, we don't have a release until it's there, so everything will be broken with a non-released clangd.

i suppose we can make use of different urls based on build configuration, but i couldn't find any existing mechanisms for doing so. hence this needs to be introduced.
another option would be to have a redirection from releases.llvm.org/15.0.0 to clang.llvm.org, until it's released, but the directory structure/subdomain is different so this will also need some adjustments.

the other option is, when we do such changes, we introduce redirections from old page schemes so that those links keep working, at least for a couple releases.

I don't think such changes will occur quite often (or ever again) in practice, so this is probably not worth the effort. but if any of these redirections are actually easy to add, we might consider doing so. as in addition to preventing breakages through such changes, it'll also make sure people are seeing the documentation for the check as it was released with clangd.

Pointing the documentation links based on build configuration and version definitely makes sense as evolving checks could easily confuse users.
Guess the idea is snapshot builds will use the in progress release notes, and release builds can point to the actual release documentation

njames93 updated this revision to Diff 441351.Jun 30 2022, 4:36 AM

Emit documentation links for the version of clangd built.

njames93 updated this revision to Diff 441352.Jun 30 2022, 4:38 AM

Fix lit cfg typo

njames93 requested review of this revision.Jun 30 2022, 4:39 AM

Any issues with this now for getting correct version?

Hmm, this version looks complicated to me.
And also fragile: downstream we have CLANG_VERSION_STRINGs that don't match upstream, Apple has their own versioning scheme, linux distros tend to do things like 6.0.1~ubuntu3...
Let me sync with @kadircet

Hmm, this version looks complicated to me.
And also fragile: downstream we have CLANG_VERSION_STRINGs that don't match upstream, Apple has their own versioning scheme, linux distros tend to do things like 6.0.1~ubuntu3...
Let me sync with @kadircet

The good news is that the ~ubuntu3 isn't part of CLANG_VERSION_STRING I think.
Bad news #1 is that it still may not match llvm.org versions: e.g. our internal distribution is "trunk", Apple's CLANG_VERSION_STRING is 10.0.1 on my machine, but it's approximately LLVM version 7.
Bad news #2 is that the documentation isn't actually available for all these versions: none of 14.0.1->14.0.5 exist, the point releases for 9-13 all have documentation but not 8.0.1. Looking at other projects, the set of docs available is inconsistent.

I don't think this substantially more reliable than just pointing at the HEAD docs, and it certainly doesn't seem "better enough" to be worth any build complexity. Can we revert to the simple version?

(I do think changing the URLs of the clang-tidy check documentation was unfortunate, and setting up server-side redirects for those would be nice to have if it's easy)

Hmm, this version looks complicated to me.
And also fragile: downstream we have CLANG_VERSION_STRINGs that don't match upstream, Apple has their own versioning scheme, linux distros tend to do things like 6.0.1~ubuntu3...
Let me sync with @kadircet

The good news is that the ~ubuntu3 isn't part of CLANG_VERSION_STRING I think.
Bad news #1 is that it still may not match llvm.org versions: e.g. our internal distribution is "trunk", Apple's CLANG_VERSION_STRING is 10.0.1 on my machine, but it's approximately LLVM version 7.
Bad news #2 is that the documentation isn't actually available for all these versions: none of 14.0.1->14.0.5 exist, the point releases for 9-13 all have documentation but not 8.0.1. Looking at other projects, the set of docs available is inconsistent.

I don't think this substantially more reliable than just pointing at the HEAD docs, and it certainly doesn't seem "better enough" to be worth any build complexity. Can we revert to the simple version?

(I do think changing the URLs of the clang-tidy check documentation was unfortunate, and setting up server-side redirects for those would be nice to have if it's easy)

That's a good point about the point releases. The best acceptable compromise would be to just point the docs to the <MAJOR_VERSION>.0.0. This should always be a valid
The only issue here is that if there was any issue relating to the documentation URL it would surface after we have already published the release.

I did raise the issue about redirects for the check documentation when the structure was changed but it wasn't implemented.

njames93 updated this revision to Diff 441499.Jun 30 2022, 1:15 PM

Changed to only use the major version.
If this still seems too flaky, happy to go back to just linking to the in progress release notes.

What's the plan with this, a fix wouod likely need to be backportsd for this.

sorry this has slipped through. since we're going to backport it, let's make it minimal and always point to in-progress release notes (so sorry for reverting back to original version of the patch).

i think having clangd point to versioned docs would be much nicer, but it seems like we don't really have precise release guarantees for docs, especially in point releases. so i think we should first figure that story out, before going down that path (i don't think not pointing to exact version is a win, we might still be stale due to backports and there isn't much value in it).

njames93 updated this revision to Diff 449046.Aug 1 2022, 9:35 AM

Revert to first version, just so we can sort out the 15.X branch.

kadircet accepted this revision.Aug 3 2022, 1:08 AM

thanks a lot, lgtm!

This revision is now accepted and ready to land.Aug 3 2022, 1:08 AM