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

Adding support for linux and also fixing the stability on builds for osx #30

Merged
merged 8 commits into from
Jan 23, 2019

Conversation

jaredwy
Copy link
Contributor

@jaredwy jaredwy commented Jan 21, 2019

This sets the resource_dir that is available in later version of clang. This makes finding system headers much easier.

This also includes the location of /usr/include so that newer versions that include it in /Library can find it.

This also updates the way that cmake finds the homebrew version of llvm since it is versioned and not symlinked in like other libs.

There are also changes to generate_test_files so that it goes looking for hyde depending on how you generate your project. It will also use verbose by default to help in debugging.

@mmha
Copy link
Contributor

mmha commented Jan 22, 2019

To me it looks like this is overcomplicating things. I also patched hyde to make it usable on Linux and wanted to the changes after some cleanup. You can compare the changes here.

I don't know what Apple Clang does, but at least open source LLVM and Clang install a CMake config, so all that needs to be done is to find_package(Clang). When the install prefix is not a default one (I guess this is the case with Homebrew?) then the CMAKE_PREFIX_PATH or Clang_ROOT/Boost_ROOT/yaml-cpp_ROOT need to be adjusted on the command line.

@jaredwy
Copy link
Contributor Author

jaredwy commented Jan 22, 2019 via email

@mmha
Copy link
Contributor

mmha commented Jan 22, 2019

Right. As you might have guessed I don't use Apple, so I can't test it there. But it looks like you can simplify the code by doing this instead:

if(APPLE)
  list(APPEND CMAKE_PREFIX_PATH "/usr/local/Cellar/llvm/" "/usr/local/Cellar/boost/" "/usr/local/Cellar/yaml-cpp/")
endif()

find_package(Clang)
find_package(Boost)
find_package(yaml-cpp)

and the rest would stay the same as with my patch. Although the execute_process(clang -print-resource-dir) is probably better than just assuming the resource dir to be present at a specific relative location.

@jaredwy
Copy link
Contributor Author

jaredwy commented Jan 22, 2019

This doesn't won't work as LLVM installs into "/usr/local/Cellar/llvm/{version}/" and doesn't symlink into /usr/local/{include|lib}. Both boost and yaml-cpp do and that is why I don't need to set CMAKE_PREFIX_PATH` or the boost_dir/yaml-dir to find them.

I just tried your branch out on osx and after I make the edits to be able to locate the clang package. I can't build hyde on OSX it can't locate a bunch of llvm/clang headers :( I'll see if I can get it to work on osx.

I am liking your approach with the configuration files so at a minimum I might ask that we can borrow that idea.

@mmha
Copy link
Contributor

mmha commented Jan 22, 2019

That's unfortunate, but yeah, there's no other way then.

You can take what you need from my branch. I wanted to create a PR with that anyways (followed by another PR to enable porting existing Doxygen comments) :)

@jaredwy
Copy link
Contributor Author

jaredwy commented Jan 22, 2019

Yup. I may try upstream the a change to create the symlink ala boost in clang/llvm this should really simplify the cmake file.

There will still be the ugly mac only include stuff though. It seems apples clang includes the path by default, where as the open source clang doesn't.
There may be another patch we could do to homebrews clang that adds the osx specific include path in as part of the build process, but unsure if that will work and is probably a way aways.

Copy link
Contributor

@fosterbrereton fosterbrereton left a comment

Choose a reason for hiding this comment

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

LGTM

@jaredwy jaredwy merged commit 57bd0ff into master Jan 23, 2019
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.

None yet

4 participants