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

FindLibClang: add version 8.0.0_1 #539

Conversation

goxberry
Copy link

This commit adds the version string "8.0.0_1" to the list of LLVM
versions because the current version of LLVM in Homebrew (as of the
time of this commit) is version 8.0.0_1.

Another approach for detecting a macOS Homebrew install of the "llvm"
package (latest stable version) would be to add /usr/local/opt/llvm
to the list of search paths. Homebrew installs of older major versions
of LLVM are symlinked to paths of the form
/usr/local/opt/llvm@${MAJOR_VERSION} (e.g.,
/usr/local/opt/llvm@7/, /usr/local/opt/llvm@6/).

@goxberry
Copy link
Author

Fixes #540.

@Sarcasm
Copy link
Owner

Sarcasm commented Jun 22, 2019

Hum,
one thing I don't understand with Homebrew.
I feel like CMakeLists.txt should not care much about Homebrew, but Homebrew should adapt to packages standard conventions.

Isn't there some common install prefix that Homebrew install all its softwares?
Does Homebrew install the ClangConfig.cmake file?
If so, where is it located?

Maybe supporting Homebrew is just a matter of specifying a -DCMAKE_PREFIX_PATH=<some/Homebrew/prefix>.
When you do which clang, what path do you get?

@eagleflo
Copy link
Contributor

ClangConfig.cmake is located in /usr/local/opt/llvm/lib/cmake/clang/ when LLVM is installed via Homebrew.

@Sarcasm
Copy link
Owner

Sarcasm commented Jun 23, 2019

Reading the Homebrew documentation, I am under the impression that each package is installed in a versioned directory under a "Cellar", but it should be symlink to /usr/local (or another customized prefix).
That's what I get from: https://brew.sh/#question

That looks like it should requires nothing from CMake, apart maybe from -DCMAKE_PREFIX_PATH=/usr/local if this path is not part of the CMAKE_SYSTEM_PREFIX_PATH.

If the LLVM/Clang package is different from that, it would be nice to know why, and what is expected to do for consumers like irony-mode?

@Sarcasm
Copy link
Owner

Sarcasm commented Jun 23, 2019

Does the build work when you use?

cmake -DCMAKE_PREFIX_PATH=/usr/local/opt/llvm <irony-args...>

@eagleflo
Copy link
Contributor

Yes, adding that prefix to the M-x irony-install-server line fixes the build issue for me (with LLVM installed from Homebrew).

You are right that Homebrew usually symlinks its packages under /usr/local, but LLVM is a special "keg-only" formula where this isn't done by default. The reasoning is that Xcode already installs Apple's fork of LLVM and Clang and having both Apple's fork and mainline LLVM / Clang in the PATH at the same time might cause trouble.

(Apple's own libclang.dylib can be found under /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/. It doesn't ship with a ClangConfig.cmake file.)

@Sarcasm
Copy link
Owner

Sarcasm commented Jun 27, 2019

Well, I would prefer a PR that adds /usr/local/opt/llvm to the search paths.
There is already quite a few hardcoded macOs paths.

But to be honest, I don't think it fits in this place anymore, and it would be better to specify -DCMAKE_PREFIX_PATH=/usr/local/opt/llvm from the command line.
That way, the ClangConfig.cmake shipped by LLVM might be found, thanks to a recent change:

find_package(Clang)

This can be done by settings irony-extra-cmake-args appropriately.

I'm not sure I will update the LIBCLANG_KNOWN_LLVM_VERSIONS variable anymore, since modern LLVM versions should provide usable CMake configs.

@goxberry
Copy link
Author

@Sarcasm I think the suggestion to set irony-extra-cmake-args is useful and more elegant than the current PR. I also suspect that the most common LLVM configuration for Homebrew would use the latest stable version of LLVM provided by Homebrew; hardcoding the search path suggested by @eagleflo would cover this case and would likely decrease support burden. As implied by your most recent post, I'd add the search paths for Clang's CMake config file to the PATHS argument of find_package(Clang) currently at irony-mode/server/src/CMakeLists.txt:4.

I'm happy to make that change, along with similar modifications for other common configurations (Debian/Ubuntu, MacPorts, FreeBSD, Gentoo), if someone else is willing to provide the appropriate paths.

@goxberry
Copy link
Author

It's also worth noting that ClangConfig.cmake is present in LLVM 3.9.0 and above. Correctly setting search paths in the find_package(Clang) call at irony-mode/server/src/CMakeLists.txt:4 would obviate the need for adding maintenance versions of recent LLVM (e.g., 7.1.0) to LIBCLANG_KNOWN_LLVM_VERSIONS.

This commit adds search paths for the `ClangConfig.cmake` config file
corresponding to the latest version of LLVM present in Homebrew.

Another approach for detecting a macOS Homebrew install of the "llvm"
package (latest stable version) would be to add `/usr/local/opt/llvm`
to the list of search paths. Homebrew installs of older major versions
of LLVM are symlinked to paths of the form
`/usr/local/opt/llvm@${MAJOR_VERSION}` (e.g.,
`/usr/local/opt/llvm@7/`, `/usr/local/opt/llvm@6/`).
@goxberry goxberry force-pushed the bugfix/add-homebrew-llvm-version-8.0.0_1 branch from 281ae60 to 5396dd3 Compare June 28, 2019 16:15
@goxberry
Copy link
Author

Changes made in 5396dd3.

@Sarcasm
Copy link
Owner

Sarcasm commented Jul 1, 2019

Hum, I'm not fond of the patch, I think this is better set externally, in the user config or eventually in some .el in irony mode (but that can come at a latter time).

Right now we could add the following, with instructions in the Wiki or README:

(setq irony-extra-cmake-args
      (list (format "-DCMAKE_PREFIX_PATH=%s" (or (getenv "HOMEBREW_PREFIX")
                                                 "/usr/local/opt/llvm"))))

I don't want to handle all the package managers out there, the benefit of find_package(Clang) over the FindLibclang.cmake, was to simplify the CMake files.

@goxberry
Copy link
Author

goxberry commented Jul 1, 2019

@Sarcasm I like your suggestion. Perhaps the code snippet should be slightly modified?

(setq irony-extra-cmake-args
      (list (format "-DCMAKE_PREFIX_PATH=%s" (concat (or (getenv "HOMEBREW_PREFIX")
                                                 "/usr/local")) "/opt/llvm"))

HOMEBREW_PREFIX will only give the Homebrew prefix install directory, which is /usr/local by default on macOS and ${HOME}/.linuxbrew by default on Linux. There could, of course, be additional logic to fall back to the default install directory that depends on the OS, but I'm not sure that additional complexity would be helpful. Assuming the snippet above is syntactically correct -- I don't write much code in Emacs Lisp -- it should cover most use cases.

@Sarcasm
Copy link
Owner

Sarcasm commented Jul 2, 2019

Ah yes you are right.
Since you use Homebrew, you can try it out to see if that is ok.

zakkak added a commit to zakkak/irony-mode that referenced this pull request Oct 9, 2019
@goxberry goxberry closed this Jul 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants