Page MenuHomePhabricator

Add CMake bits necessary for standalone build
ClosedPublic

Authored by krytarowski on Nov 29 2015, 9:14 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

krytarowski retitled this revision from to Design building out of sources.
krytarowski updated this object.
krytarowski added a reviewer: labath.
krytarowski set the repository for this revision to rL LLVM.
krytarowski added subscribers: lldb-commits, joerg.

@labath feel free to disagree with the regex files. It was copied from LLVM. If you disagree, help me please to export it in LLVM build for everybody or find a better solution.

pkgsrc-wip commit affirming that it works:
https://wip.pkgsrc.org/cgi-bin/gitweb.cgi?p=pkgsrc-wip.git;a=commit;h=85621c794d26ed60844215ae16d3b4a4309d969e

Without this patch I'm falling asleep in front of the computer waiting for build to be finished.

Upload the proper latest patch.

labath edited edge metadata.Nov 30 2015, 2:40 AM
labath added subscribers: artagnon, Eugene.Zelenko, tfiala.

It seems to follow what clang does here, so I think it should be ok.

However, I'm not actually using the standalone build... I've added some people who I think might be using it. Let's give them a bit of time to respond. If there are no objections, then I'll commit this..

zturner requested changes to this revision.Nov 30 2015, 8:35 AM
zturner added a reviewer: zturner.
zturner added a subscriber: zturner.

@labath, can you point out where clang is doing the same thing? I searched for files named regcomp.c for example, and only found the one copy in llvm. Duplicating code seems like a blocking issue to me, this will make the THIRD regex implementation in LLDB if we do this.

Standalone build does not mean you don't need to link against LLVM libraries.

This revision now requires changes to proceed.Nov 30 2015, 8:35 AM

Hmm.. you are right, of course.. I was only looking at the second version of the patch, which seems to omit the regex files. However that seems to by accidental, as they are still references in the CMake files.

I agree that we should not copy the files over.

@krytarowski, I assume that the problem is that the regex headers are in llvm/lib (instead of include/llvm), which means they are not installed, and you cannot include them later. Is that correct?

It seems that is being done on purpose, so I guess it's a question whether lldb-mi should have been using them in the first place. We'll need to think about this more.

lldb-mi is including from the source tree, it should be including from the incldue tree.

In other words, the incldue should be changed to:

#include "llvm/Support/Regex.h"

and the code should be updated to use llvm::Regex instead of the raw c functions.

labath requested changes to this revision.Nov 30 2015, 9:06 AM
labath edited edge metadata.

OK, llvm seems to export the regex interface via include/llvm/Support/Regex.h. I think we should fix lldb-mi to use this interface instead, and then our problem will go away. The MI wrapper seems to be very simple, so it should not be a big problem to port it over.

I build LLVM/Clang with LLVM_ENABLE_WARNINGS=ON, so I'll need to use llvm-config from build directory. I'm not sure how well find_program will work if path to <build directory>/bin will be not added to PATH, so probably will be good idea to allow LLVM_CONFIG to be set in Cmake command line and use find_program only if LLVM_CONFIG is not set.

I build LLVM/Clang with LLVM_ENABLE_WARNINGS=ON, so I'll need to use llvm-config from build directory. I'm not sure how well find_program will work if path to <build directory>/bin will be not added to PATH, so probably will be good idea to allow LLVM_CONFIG to be set in Cmake command line and use find_program only if LLVM_CONFIG is not set.

This should be already handled by CMake:
https://cmake.org/cmake/help/v3.0/command/find_program.html

I build LLVM/Clang with LLVM_ENABLE_WARNINGS=ON, so I'll need to use llvm-config from build directory. I'm not sure how well find_program will work if path to <build directory>/bin will be not added to PATH, so probably will be good idea to allow LLVM_CONFIG to be set in Cmake command line and use find_program only if LLVM_CONFIG is not set.

This should be already handled by CMake:
https://cmake.org/cmake/help/v3.0/command/find_program.html

But it's necessary to pass path from CMake command line argument to find_program after PATHS. Another problem if default OS location (if Clang installed and of old version) will be returned instead of build one.

labath added a comment.Dec 1 2015, 1:50 AM

But it's necessary to pass path from CMake command line argument to find_program after PATHS. Another problem if default OS location (if Clang installed and of old version) will be returned instead of build one.

I don't know what would be the idiomatic cmake-fu to achieve that, but I'd just pass the the full path to the llvm-config binary, including the name. This way, we could also handle cases when the binary has a version suffix (e.g., debian likes to ship the binary as llvm-config-3.x).

But it's necessary to pass path from CMake command line argument to find_program after PATHS. Another problem if default OS location (if Clang installed and of old version) will be returned instead of build one.

I don't know what would be the idiomatic cmake-fu to achieve that, but I'd just pass the the full path to the llvm-config binary, including the name. This way, we could also handle cases when the binary has a version suffix (e.g., debian likes to ship the binary as llvm-config-3.x).

I think find_program() should be called if LLVM_CONFIG is empty or not exist.

For the reference, I hold on with pushing this forward, till I will get significantly larger coverage with tests on my platform.

I keep this patch locally for local development.

Just in case someone will be quicker, new version against revamped CMake dirs is in pkgsrc-wip/lldb-git

labath resigned from this revision.Mar 15 2016, 1:41 AM
labath removed a reviewer: labath.

I am cleaning out my review queue. Feel free to add me back when you want to get this moving again...

krytarowski updated this revision to Diff 55742.May 1 2016, 4:38 AM
krytarowski edited edge metadata.

Revamp the patch and reduce it only to the CMake part.

Leave regex code for later.

krytarowski added inline comments.May 1 2016, 5:09 AM
cmake/modules/LLDBStandalone.cmake
85

When I am including conditionally include(FindPythonInterp) with this PYTHON_EXECUTABLE STREQUAL "" conditional I'm facing the said issue with:

CMake Error at scripts/cmake_install.cmake:31 (file):
  file INSTALL cannot find
  "/tmp/pkgsrc-tmp/wip/lldb-git/work/build/lib/python.".
Call Stack (most recent call first):
  cmake_install.cmake:37 (include)
  


*** Error code 1

If I put here include(FindPythonInterp) unconditionally, there is no error in cmake_install.cmake:37.

krytarowski added inline comments.May 1 2016, 2:24 PM
cmake/modules/LLDBStandalone.cmake
85

My solution to this issue was to include include(FindPythonInterp) in scripts/CMakeLists.txt as noted in D19772.

labath accepted this revision.May 3 2016, 1:29 AM
labath edited edge metadata.

Looks good.

Looks good.

Thanks!

Are you accepting it with the patch in scripts/CMakeLists.txt as noted in the comment?

labath added a comment.May 3 2016, 8:28 AM

Good question. I don't really have an opinion on that... Is it just that single line? (this would be simpler if you uploaded the final version :) ). If it just that single line, then I think it's fine. If you also need to play with the ADDITIONAL_VERSIONS and such, then we should think about making a macro or doing something else to avoid code duplication (there is already a FIXME in scripts/Python/modules/readline mentioning the additional versions).

Good question. I don't really have an opinion on that... Is it just that single line? (this would be simpler if you uploaded the final version :) ). If it just that single line, then I think it's fine. If you also need to play with the ADDITIONAL_VERSIONS and such, then we should think about making a macro or doing something else to avoid code duplication (there is already a FIXME in scripts/Python/modules/readline mentioning the additional versions).

Sorry for delay. I'm going to update the diff.

krytarowski retitled this revision from Design building out of sources to Add CMake bits necessary for standalone build.
krytarowski edited edge metadata.

Updated. Can I commit it this way as it is?

I would like to see implemented my comment about LLVM_CONFIG and find_program().

I would like to see implemented my comment about LLVM_CONFIG and find_program().

Could you first push this to clang? I don't want to drift from clang myself.

I would like to see implemented my comment about LLVM_CONFIG and find_program().

Could you first push this to clang? I don't want to drift from clang myself.

I'm not sure about Clang make files, but I built Clang with LLVM. LLDB is more problematic, so I build it standalone with previously build Clang. This why I'd like to specify path in CMake parameters, not relying in find_program().

labath accepted this revision.May 12 2016, 3:15 AM
labath edited edge metadata.

I would like to see implemented my comment about LLVM_CONFIG and find_program().

Could you first push this to clang? I don't want to drift from clang myself.

I'm not sure about Clang make files, but I built Clang with LLVM. LLDB is more problematic, so I build it standalone with previously build Clang. This why I'd like to specify path in CMake parameters, not relying in find_program().

From cmake documentation on find program:

This command is used to find a program. A cache entry named by <VAR> is created to store the result of this command. If the program is found the result is stored in the variable and the search will not be repeated unless the variable is cleared.

So, I think the functionality you request is already baked into find_program and we should not duplicate it. I made a quick test and setting the variable via -DLLVM_CONFIG on the cmake command line does indeed prevent find-program from doing anything. (which is not surprising, as it is a fairly common use case for people to want to specify their own paths to things...)

I think this is good to go know. I don't think we need to wait for @zturner, as his objections were about the regular expression thing, which has been removed now.

@Eugene.Zelenko do you agree with this patch and what @labath wrote?

@Eugene.Zelenko do you agree with this patch and what @labath wrote?

If CMake works that way, it's what I need :-)

This revision was automatically updated to reflect the committed changes.