Hi Alex, hi Pavel Thanks for your feedback and sorry for the late reply, I was a little busy on the other sites.
Ok, so making the LLDB_FRAMEWORK_INSTALL_DIR vs. LLDB_FRAMEWORK_BUILD_DIR distinction and changing the approach for tools from symlink to copy sound like good tasks to start with. I will see how far I get without changing the processing order. I am a little afraid about Pandora's box here. Anyway, thanks for the explanation and proposal! Had a look at the history and it turned out the reason for the 3.7 requirement was a bug in CMake (https://gitlab.kitware.com/cmake/cmake/issues/16363). There was a workaround, but it was dropped (https://reviews.llvm.org/rLLDB289841). Policy-wise CMake always acts as if it was cmake_minimum_required, so I am always sceptic about version checks, but for a bug like this it seems fine. Let’s keep it as long as cmake_minimum_required is below. Best Stefan > On 10. Nov 2018, at 11:22, Pavel Labath <pa...@labath.sk> wrote: > > Hello Stefan, > > first off, I want to point out that although I spent a lot of time > digging through our cmake files, I am not using the framework build, nor > am familiar with it's details. > > I also don't know why the framework was split into multiple targets. > What I do know is that we then ended up adding extra indirections so > that we can make setting up dependencies easier (lldb can just depend on > ${LLDB_SUITE_TARGET} without knowing whether framework was enabled or > not). If you can merge everything into a single target, then this > indirection could also go away. > > Your ideas sound reasonable to me. As Alex points out, centralizing > everything to API/CMakeLists.txt may be tricky, although I agree that is > the natural place to do this sort of thing. The trickyness comes from > the fact that our cmake files are not processed in dependency order. > Although this would be somewhat unconventional, I wouldn't be opposed to > changing the cmake processing order so that it more closely matches the > dependencies. If that makes things easier for you, that is. > > One way to achieve this would be to take out source/API and > tools/debugserver invocations out of source and tools CMakeLists.txt > files, respectively. and put them into the top level file. So then the > top level file would do something like: > ``` > add_subdirectory(source) # Build all consituent libraries > > # Tools going into the lldb framework > add_subdirectory(tools/debugserver) > add_subdirectory(tools/argdumper) > ... > > add_subdirectory(source/API) # Build LLDB.framework (or liblldb) itself > add_subdirectory(tools) # Build tools that depend on the framework > ``` > > This could be cleaned up if we moved the source code around, but I'm not > sure we want to take that hit (I don't think it's worth it). Also, it's > just an idea, I don't know whether that will make your job easier or not. > > Good luck, > pavel > > On 09/11/18 21:31, Alex Langford wrote: >> Hi Stefan, >> >> Thanks for taking the time to improve LLDB's CMake infrastructure! >> >> (1) I don't entirely remember the reason I had separated them out into >> separate targets. A post-build step would be fine, so feel free to merge >> these. >> (2) I have no problems with this. If I remember correctly, I wanted to put >> everything into LLDBFramework.cmake originally and include it if >> LLDB_BUILD_FRAMEWORK got set, but that proved more difficult than I >> realized. I think what you are suggesting is better than what I ended up >> doing. Feel free to make that change. >> (3) I think this is a good idea. I found this confusing as well. >> (4) If that is the case, I think I agree that the Xcode behavior is better. >> (5) I'm not sure that this is going to actually simplify things. I don't >> think it's possible to do in source/API/CMakeLists.txt because the tools >> haven't been added as targets yet. With the current CMake logic, you could >> pull this logic into its own section that happens after the tools are added >> as targets. I don't find this any better than what we have now. >> (6) I'm not sure why this is the case actually. I believe this was added by >> beanz originally, I just moved this check to be closer to the beginning of >> the build. If it works with CMake versions less than 3.7 then I have no >> issues with removing it. >> >> Let me know if something is unclear or you have further questions/concerns. >> >> Alex >> >> >> On 11/9/18, 9:38 AM, "sgraen...@apple.com on behalf of Stefan Gränitz" >> <sgraen...@apple.com> wrote: >> >> Hello Alex, hello Pavel >> >> I spent some time creating/streamlining our CMake infrastructure >> downstream of LLDB and learned a few things about bundles, versions, >> code-signing etc. (mostly on the Darwin side of things). I am currently >> sorting out what can be upstreamed and prepare reviews. >> >> Some work is still todo for the LLDB shared library/framework (for >> simplicity I will call it LLDB.framework). It would be great to know, if you >> have concerns or comments on the following points: >> >> (1) The liblldb target builds the actual LLDB.framework, while the >> lldb-framework target adds headers, symlinks, etc. What is the reason for >> this separation? Can I merge that into one target with post-build steps? >> >> (2) In previous reviews there was an effort to centralize the code for >> building LLDB.framework, which makes sense to me. With the current >> LLDBFramework.cmake approach, however, it’s spread over at least 3 different >> files (lldb/CMakeLists.txt for init and lldb/source/API/CMakeLists.txt for >> actual definition). In a similar case downstream, I did all that in a single >> CMakeLists.txt in the source folder. While I see that LLDBFramework affects >> the whole project, I don’t see why we need a separate LLDBFramework.cmake >> (BTW upstream it’s included only once). Do you think I can move things to >> lldb/source/API/CMakeLists.txt where possible? >> >> (3) Currently the build directory for LLDB.framework is determined from >> LLDB_FRAMEWORK_INSTALL_DIR, which I think is a little confusing. Can I clean >> this up? (e.g. having a LLDB_FRAMEWORK_BUILD_DIR) >> >> (4) With Xcode, executables are emitted in bin and copied to >> LLDB.framework where necessary. CMake emits them into LLDB.framework >> directly and creates symlinks to bin. With LLVM_EXTERNALIZE_DEBUGINFO on >> Darwin, this has the effect, that by default their dSYMs will end up in >> LLDB.framework. Thus, I would prefer the Xcode behaviour here. >> >> (5) Couldn’t (4) also simplify the INCLUDE_IN_SUITE logic? I would >> consider it to be LLDB.framework’s responsibility to set dependencies and >> adjust RPATHs for all required artefacts. The tools wouldn’t need to care >> about that (though, they could still check LLDB_BUILD_FRAMEWORK). The >> RPATH-login for case ARG_INCLUDE_IN_SUITE && LLDB_BUILD_FRAMEWORK is quite >> complicated though and I wonder if there are strong reasons not to do that. >> What do you think? >> >> (6) Just out of interest: why is LLDB_BUILD_FRAMEWORK is not supported on >> CMake < 3.7? >> >> Best >> Stefan >> >> >> >> >> > _______________________________________________ lldb-dev mailing list lldb-dev@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
