Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[cpp] Update clangd install snippet to clangd-10 in README.md #6271

Merged
merged 1 commit into from
Oct 4, 2019

Conversation

jankeromnes
Copy link
Member

@jankeromnes jankeromnes commented Sep 26, 2019

Because that's what's in the 'llvm-toolchain-bionic' repository now: https://apt.llvm.org/

Thanks to @sylvestre for catching the outdated snippet!

What it does

Fixes the clangd install snippet in the default C++ extension's README.md.

How to test

Run the snippet in a fresh Ubuntu 18.04 Bionic install (e.g. using the ubuntu:bionic Docker image).

Review checklist

Reminder for reviewers

@akosyakov akosyakov added the cpp issues related to the C/C++ language label Sep 27, 2019
@simark
Copy link
Contributor

simark commented Sep 27, 2019

clang 9 has just been released, which means it's the current stable. clang 10 is the new development branch, not the stable one. So I believe we should drive people towards clang 9.

@sylvestre
Copy link

If you want to use the 9 branch, the repo should be llvm-toolchain-bionic-9 and not llvm-toolchain-bionic
llvm-toolchain-bionic only gets upstream master, so, 10 currently.

@jankeromnes
Copy link
Member Author

jankeromnes commented Sep 27, 2019

@sylvestre Good point.

@simark According to https://apt.llvm.org/, clangd 8 is actually the "stable" branch, while clangd 9 is the "qualification" branch (and clangd 10 is the "development" branch).

We could install clangd-8 (stable EDIT: old stable) like so:

$ echo "deb http://apt.llvm.org/bionic/ llvm-toolchain-bionic-8 main" | sudo tee /etc/apt/sources.list.d/llvm.list
$ sudo apt-get update && sudo apt-get install -y clang-tools-8
$ sudo ln -s /usr/bin/clangd-8 /usr/bin/clangd

Or clangd-9 (qualification EDIT: stable) like so:

$ echo "deb http://apt.llvm.org/bionic/ llvm-toolchain-bionic-9 main" | sudo tee /etc/apt/sources.list.d/llvm.list
$ sudo apt-get update && sudo apt-get install -y clang-tools-9
$ sudo ln -s /usr/bin/clangd-9 /usr/bin/clangd

But these require version suffixes that will break after each LLVM release.

Alternatively, we can also install clangd-10 (development) as clangd (without any suffix anywhere), simply like so:

$ echo "deb http://apt.llvm.org/bionic/ llvm-toolchain-bionic main" | sudo tee /etc/apt/sources.list.d/llvm.list
$ sudo apt-get update && sudo apt-get install -y clang-tools

I believe that LLVM's development branch is considered "stable enough" by major actors and projects like Chromium and Firefox, so it's unlikely to break in catastrophic ways (or at least, it wouldn't remain broken for long).

Plus we could avoid having to switch suffixes when they become deprecated every few months. In any case, I'm happy to update this PR according to your preference.

@sylvestre
Copy link

@simark According to https://apt.llvm.org/, clangd 8 is actually the "stable" branch, while clangd 9 is
the "qualification" branch (and clangd 10 is the "development" branch).

Actually, 8 is old stable, 9 is stable quand 10 is dev.
I updated the webpage to reflect that.

@marcdumais-work
Copy link
Contributor

@jankeromnes @sylvestre @simark thanks for this PR and thoughtful comments.

I notice that our clangd install instructions are quite fragile, needing update at every "crank of the clangd release wheel".

Maybe a better approach would be to make this section more generic by removing the mention of a specific Ubuntu version and pointing to instructions being updated by the clangd project. This one would be good, covering Windows and Mac too, except it instructs to install the the clangd version available in the Ubuntu 18.04 repo, which is quite old (8.x) and does not have the built-in clang-tidy.

Alternatively or additionally this page, mentioned above, would work, with a mention to add a symlink named clangd pointing to whatever version of the executable is installed.

We can still make a recommendation about which version to use; 9.x is potentially more stable than 10.x and has the built-in clang-tidy that we support, so it looks like a good one, until we find that 10.x or later has specific features we want.

WDYT?

@jankeromnes
Copy link
Member Author

jankeromnes commented Sep 30, 2019

Thanks for your helpful thoughts @marcdumais-work!

I notice that our clangd install instructions are quite fragile, needing update at every "crank of the clangd release wheel".

That's true currently, but it's not the case with the latest suggestion:

$ echo "deb http://apt.llvm.org/bionic/ llvm-toolchain-bionic main" | sudo tee /etc/apt/sources.list.d/llvm.list
$ sudo apt-get update && sudo apt-get install -y clangd

This snippet will always pick the latest clangd version. There is no need to change anything when clangd makes a new release, nor to manually maintain a special symlink.

Also, as previously mentioned, using the latest clangd is considered "stable enough" by major projects that compile a lot of C++ code every day, like Chromium and Firefox. So I personally don't think we need to make efforts to recommend older versions of clangd.

Still, if you prefer recommending 9.x for now, we could indeed make the instructions more generic by pointing to https://clang.llvm.org/extra/clangd/Installation.html#installing-clangd and/or to https://apt.llvm.org/, and by explaining how to maintain the special symlink.

@marcdumais-work
Copy link
Contributor

This snippet will always pick the latest clangd version. There is no need to change anything when clangd makes a new release, nor to manually maintain a special symlink.

👍 Perfect then, thanks for the clarification

So I personally don't think we need to make efforts to recommend older versions of clangd.

👍

Still, if you prefer recommending 9.x for now,

No, I think it's ok, as long as this documentation is "low maintenance" and we pick-up a version that has support for our integrations, which latest does.

Copy link
Contributor

@marcdumais-work marcdumais-work left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tested by switching to the clangd package on my Ubuntu 18.04 machine (I had clangd-9 from same PPA before)

@akosyakov
Copy link
Member

@marcdumais-work please merge @jankeromnes is not a committer

@marcdumais-work marcdumais-work merged commit 4a07198 into eclipse-theia:master Oct 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cpp issues related to the C/C++ language
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants