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

Added check for /usr/lib64 for fix issue in Fedora 33 #31

Merged
merged 1 commit into from
Jul 1, 2021

Conversation

andrewxcav
Copy link
Contributor

Effects:

  • If your initial call to cmake worked before it should still work
  • If you had the same issue I had with the existence of /lib64 this fixes it
  • CMakeLists.txt is a bit more cluttered though

I think this will lower a significant barrier to folks who are going to install the llvm-devel package but not necessarily build llvm-project from source.

@banach-space
Copy link
Owner

Hi @andrewxcav ,

Many thanks for looking into this and submitting a fix! I didn't realise that there were issues on Fedora - good catch! I've just created #32 and will try to set something up to better defend against things like this in the future. Volunteers are always very welcome ;-)

Note that your change will only silence the CMake error. I think that you also need to update:

I suspect that find_package(LLVM REQUIRED CONFIG) works for you anyway as your LLVM installation is in some "standard" location (i.e. "auto-magically" included in CMake's search path).

Last, but not least - would you mind adding a short note in CMake scripts explaining the reasoning behind duplicating set(LT_LLVM_CMAKE_FILE ...)? The commit message explains it really well, but it's a rather tricky and IMHO worth documenting in as many places as possible.

Thanks again for submitting this!

-Andrzej

PS BTW, I am about to update llvm-tutor to LLVM 12, which should fix the CI failure.

@banach-space
Copy link
Owner

@andrewxcav , I've just updated llvm-tutor to LLVM 12. Could you re-base, please? I hope that this goes smoothly for you! Let me know if you hit any problems.

@banach-space
Copy link
Owner

This thread has gone a bit stale :)

This is a much needed change and I will merge it as is! With the new Fedora docker set-up, I will be able to test and fine-tune it (if required).

Thank you @andrewxcav !

@banach-space banach-space merged commit a139213 into banach-space:main Jul 1, 2021
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.

2 participants