This is an archive of the discontinued LLVM Phabricator instance.

[cmake] Slight fix ups to make robust to the full range of GNUInstallDirs
ClosedPublic

Authored by Ericson2314 on Jul 25 2022, 9:30 PM.

Diff Detail

Event Timeline

Ericson2314 created this revision.Jul 25 2022, 9:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 25 2022, 9:30 PM
Herald added a subscriber: mgorny. · View Herald Transcript
Ericson2314 requested review of this revision.Jul 25 2022, 9:30 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 25 2022, 9:30 PM

Add another fix

Herald added a project: Restricted Project. · View Herald Transcript
sebastian-ne accepted this revision.Jul 26 2022, 2:54 AM

Looks good to me

openmp/CMakeLists.txt
3–9

There are some places that surround this by an if(NOT DEFINED LLVM_COMMON_CMAKE_UTILS) (e.g. lldb/cmake/modules/LLDBStandalone.cmake, clang/CMakeLists.txt, polly/CMakeLists.txt) although other places don’t (lld/CMakeLists.txt, libunwind/CMakeLists.txt, etc.), so both seem to be fine.

This revision is now accepted and ready to land.Jul 26 2022, 2:54 AM
Ericson2314 added inline comments.Jul 26 2022, 7:48 AM
openmp/CMakeLists.txt
3–9

Ah true. Yeah it should be fine either way.

This revision was landed with ongoing or failed builds.Jul 26 2022, 7:49 AM
This revision was automatically updated to reflect the committed changes.
Ericson2314 added a comment.EditedJul 26 2022, 9:26 AM

Hmm I am getting failed builds from this one but not quite sure what the problem is.

Will revert if I don't figure out soon.

Same, it broke apt.llvm.org

It is installing some of the libs in debian/tmp/libgomp.so instead of debian/tmp/usr/lib/llvm-15/lib/libgomp.so

I think it should be indeed revert (and in the branch 15 too)

sebastian-ne added inline comments.Jul 28 2022, 5:26 AM
openmp/runtime/src/CMakeLists.txt
383

The backslash before \${outdir} is wrong here, it must be \"\$ENV{DESTDIR}${outdir}\")").

Also, before this patch, there were a lot of backslashes before e.g. \${CMAKE_INSTALL_PREFIX}, but I hope that shouldn’t matter, the value should be the same at configure and install time.

I pushed a potential fix (removing the backslash) in 50716ba2b337afe46ac256cc91673dc27356a776.
I don’t know which buildbots to look at though, it looks like the OpenMP ones all succeed.

PS: I’ll be away for a few weeks, please revert my fix if it causes problems.

@sebastian-ne Thanks. Your commit 100% makes it better than it was (straight typo on my part), so I cherry-picked it to release/15.x: 2647f7274782e4a529d7c3a9920de86ddfe72324

@sylvestre.ledru That fix is is exactly consistent with the bug you saw. I have stopped getting build failure emails I think that means things have been fixed? Notwithstanding the policy, I am admittedly hesitant to revert unless we are sure issues persist as I have spent the last 1.5 years trying to upstream this from my distro (NixOS) amidst a bunch of volunteer build system cleaning up.

This patch breaks the assumption that some projects are supposed to support to be built as a standalone project. For example, we do have a standalone release for OpenMP (https://github.com/llvm/llvm-project/releases/download/llvmorg-16.0.2/openmp-16.0.2.src.tar.xz). This patch uses relative path ${CMAKE_CURRENT_SOURCE_DIR}/../cmake which assumes openmp directory is inside llvm-project.