search

sourcegraph / lsif-clang

Language Server Indexing Format (LSIF) generator for C, C++ and Objective C
https://lsif.dev/
34 stars 8 forks source link

Should this repository be a fork of github.com/llvm/llvm-project? #23

Closed beyang closed 3 years ago

beyang commented 4 years ago

This project currently only appears to build against llvm version 10.

On macOS, I've tried building with llvm 9 and llvm 11 on macOS and both have failed with compiler errors (brew has a formula for llvm 9 and 11 on macOS, but none for 10). One such compiler error was:

/Users/beyang/src/github.com/sourcegraph/lsif-clang/src/FindTarget.cpp:368:29: error: no template named 'TypeVisitor'; did you mean 'TypeLocVisitor'?

That file does not #include "clang/AST/TypeVisitor.h explicitly. Rather it assumes the other #includes will pull that file in transitively. In the llvm 10, it does; in llvm 11, it does not.

This repository began life as a fork of clangd, whose source lives in a subdirectory of the main llvm project.

Given that clangd has a source-to-source dependency on the rest of LLVM and is versioned with it, should this repository do the same? This may also help with pulling in upstream changes from the main LLVM source. (This may also help with eventually upstreaming LSIF generation back into the main LLVM project, but this is a secondary consideration.)

shrouxm commented 4 years ago

Right now the repo is versioned with LLVM 10, that's why the main branch is llvmorg-10.0.0-lsif-clang. It might make sense to maintain that going forward, I haven't tried to support any other versions yet so I'm not sure what it would look like to try to do that via include directives.

I'm not sure why brew dropped LLVM 10, I went back to my notes from when we got everything working on MacOS and it worked because brew just installed version 10. Now that 11 is out, it seems they provide 7, 8, 9, and 11? I wonder if someone just fudged the upgrade?

shrouxm commented 4 years ago

Looked into this a bit more, seems that when the llvm formula got upgraded to 11 (see commit history of https://github.com/Homebrew/homebrew-core/blob/master/Formula/llvm.rb), no one created a corresponding llvm@10.rb formula in the same directory (see e.g. https://github.com/Homebrew/homebrew-core/blob/master/Formula/llvm@9.rb). I don't see any reason that copying the source of llvm.rb at the commit before it was upgraded to llvm 11 into an llvm@10.rb wouldn't be the right move, but I can't test the result without Apple hardware.

Would you be able to do that and open the corresponding PR @beyang? There are instructions here: https://github.com/Homebrew/homebrew-core/blob/master/CONTRIBUTING.md.

This doesn't answer the general question of what to do about supporting different LLVM versions, but does provide a quick fix for the current issue I think.

efritz commented 4 years ago

Apparently it was intentional on the part of brew maintainers: https://github.com/Homebrew/homebrew-core/pull/63938

tjdevries commented 3 years ago

This is effectively a fork of llvm-project now -- instructions & rationale are documented in tree.