[llvm] r341045 - Revert "[CMake] Use LLVM_ENABLE_IDE instead of CMAKE_CONFIGURATION_TYPES"
Roman Lebedev via llvm-commits
llvm-commits at lists.llvm.org
Tue Oct 9 13:10:01 PDT 2018
On Tue, Oct 9, 2018 at 10:58 PM Chris Bieneman <chris.bieneman at me.com> wrote:
>
> (+EricWF since he originally authored LLVM_ENABLE_IDE)
>
> Here's what I think needs to be done:
>
> (1) Remove the use of LLVM_ENABLE_IDE from LLVMProcessSources.cmake
>
> That code shouldn't be conditional anyways. Setting `HEADER_FILE_ONLY` on sources makes all the non-IDE generators ignore it anyways. That said, we do probably always want all our source files properly listed on each target because the cmake-server provides that information to any tools using it.
> (2) Change the default value of LLVM_ENABLE_IDE to be based off `CMAKE_CONFIGURATION_TYPES`
>
> This would default LLVM_ENABLE_IDE to `On` whenever you are using a generator that generates more than one build configuration in a single go. Specifically Visual Studio, Xcode, and the newly added Green Hills generator. Using `CMAKE_CONFIGURATION_TYPES` instead of `XCODE OR MSVC_IDE`, would allow us to be slightly future proof on new generators.
>
> We should not have the default value be based on `CMAKE_EXTRA_GENERATOR` because that actually shouldn't matter.
I do not follow.
https://cmake.org/cmake/help/v3.0/variable/CMAKE_EXTRA_GENERATOR.html
> When using the Eclipse, CodeBlocks or KDevelop generators, CMake generates Makefiles (CMAKE_GENERATOR)
> and additionally project files for the respective IDE.
> This IDE project file generator is stored in CMAKE_EXTRA_GENERATOR (e.g. “Eclipse CDT4”).
The presence of CMAKE_EXTRA_GENERATOR is, as far as i can see, exactly
what you want to be checking to detect IDE.
How is CMAKE_CONFIGURATION_TYPES relevant here?
> (3) Change the behavior of LLVM_ENABLE_IDE to only gate on *optional* behaviors that should be disabled by default for IDEs
>
> After step (1) the LLVM_ENABLE_IDE variable becomes unused, so we can re-use it for anything that makes sense. In this case, I think what makes sense is to have it control behaviors like optional target generation that we currently disable for IDE generators due to limitations in the IDEs.
>
> If we make these changes this will allow QtCreator, SublimeText, CLion, and all the other "extra generator" formats to work correctly, while allowing a manual override to get the IDE-like behavior for Visual Studio's CMake integration.
>
> Thoughts?
How will this not regress all the IDE users that want those targets to
exist, again?
> -Chris
Roman.
> On Oct 9, 2018, at 12:43 PM, Roman Lebedev <lebedev.ri at gmail.com> wrote:
>
> On Tue, Oct 9, 2018 at 10:35 PM Chris Bieneman <chris.bieneman at me.com> wrote:
>
>
>
>
> On Oct 9, 2018, at 11:48 AM, Roman Lebedev <lebedev.ri at gmail.com> wrote:
>
> On Tue, Oct 9, 2018 at 9:41 PM Chris Bieneman <chris.bieneman at me.com> wrote:
>
>
>
>
> On Aug 30, 2018, at 2:32 AM, Roman Lebedev via llvm-commits <llvm-commits at lists.llvm.org> wrote:
>
> Author: lebedevri
> Date: Thu Aug 30 02:32:09 2018
> New Revision: 341045
>
> URL: http://llvm.org/viewvc/llvm-project?rev=341045&view=rev
> Log:
> Revert "[CMake] Use LLVM_ENABLE_IDE instead of CMAKE_CONFIGURATION_TYPES"
>
> That resulted in the check-llvm-* targets not being avaliable
> in the QtCreator-configured build directories.
>
> Moreover, that was a clearly non-NFC change, and i can't find any review
> for it.
>
>
> And no discussion on the revert either?
>
> This wasn't intended to be an NFC change. I apologize if it caused an issue with QtCreator, but a discussion would have been warranted before the revert.
>
> I've been making lots of non-NFC changes to CMake without review for years, which is consistent with the developer policy (See point #4 https://www.llvm.org/docs/DeveloperPolicy.html#code-reviews). This was a fairly small and isolated change. Again, I apologize that it caused you problems.
>
> The problem this was trying to address is that Visual Studio doesn't handle the target explosion caused by these options well, so using LLVM_ENABLE_IDE to filter them down makes sense. Obviously you care about those targets existing in QTCreator, but are setting LLVM_ENABLE_IDE.
>
> Completely hiding build targets is not a good idea.
> That option should improve expirience of the IDE users, but not
> degrade the expirience
> of those who use IDE plsu directly operate in the command-line with
> the build configured by IDE.
>
>
> Actually, disabling the target generation is exactly what we need. Both Visual Studio and Xcode have drop-down menus that are populated with targets and have maximum sizes that are too small to display all the LLVM targets. Excluding the generation of these targets results in more manageable IDE experiences.
>
> Well, i guess you want to add a new *opt-in* cmake option then.
>
> Presently this is handled by the `CMAKE_CONFIGURATION_TYPES` check, however that doesn't work with Visual Studio 2017's CMake integration which generates a ninja build, but populates the IDE with the output from the CMake server.
>
>
> I suspect the actual solution to the problem you're having is that we need to change how LLVM_ENABLE_IDE is defaulted. It should likely default `On` only for Xcode and Visual Studio generators, and it should probably default `Off` for `CMAKE_EXTRA_GENERATOR`.
>
> No. That is the exact opposite from what the current LLVM_ENABLE_IDE
> was implemented for.
> https://reviews.llvm.org/D40219
>
> The other issue is that the current use of `LLVM_ENABLE_IDE` in LLVMProcessSources.cmake probably doesn't need to be there at all. I'm fairly certain everything it is doing there is safe to do regardless of the generator, and should just be done by default.
>
> Don't know, maybe.
>
> -Chris
>
> Roman.
>
> Perhaps you meant to use https://cmake.org/cmake/help/v3.0/prop_tgt/FOLDER.html
>
> Are you setting LLVM_ENABLE_IDE deliberately?
>
> No, it is autodetected.
>
> I'd like to come to a solution that works for your needs as well as my own.
>
> -Chris
>
> Roman.
>
> This reverts commit rL340435.
>
> Modified:
> llvm/trunk/CMakeLists.txt
> llvm/trunk/cmake/modules/AddLLVM.cmake
> llvm/trunk/cmake/modules/CMakeLists.txt
> llvm/trunk/tools/xcode-toolchain/CMakeLists.txt
>
> Modified: llvm/trunk/CMakeLists.txt
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/CMakeLists.txt?rev=341045&r1=341044&r2=341045&view=diff
> ==============================================================================
> --- llvm/trunk/CMakeLists.txt (original)
> +++ llvm/trunk/CMakeLists.txt Thu Aug 30 02:32:09 2018
> @@ -978,7 +978,7 @@ if (NOT LLVM_INSTALL_TOOLCHAIN_ONLY)
> add_custom_target(llvm-headers DEPENDS intrinsics_gen)
> set_target_properties(llvm-headers PROPERTIES FOLDER "Misc")
>
> - if (NOT LLVM_ENABLE_IDE)
> + if (NOT CMAKE_CONFIGURATION_TYPES)
> add_llvm_install_targets(install-llvm-headers
> DEPENDS llvm-headers
> COMPONENT llvm-headers)
> @@ -988,7 +988,7 @@ endif()
> # This must be at the end of the LLVM root CMakeLists file because it must run
> # after all targets are created.
> if(LLVM_DISTRIBUTION_COMPONENTS)
> - if(LLVM_ENABLE_IDE)
> + if(CMAKE_CONFIGURATION_TYPES)
> message(FATAL_ERROR "LLVM_DISTRIBUTION_COMPONENTS cannot be specified with multi-configuration generators (i.e. Xcode or Visual Studio)")
> endif()
>
>
> Modified: llvm/trunk/cmake/modules/AddLLVM.cmake
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/cmake/modules/AddLLVM.cmake?rev=341045&r1=341044&r2=341045&view=diff
> ==============================================================================
> --- llvm/trunk/cmake/modules/AddLLVM.cmake (original)
> +++ llvm/trunk/cmake/modules/AddLLVM.cmake Thu Aug 30 02:32:09 2018
> @@ -659,7 +659,7 @@ macro(add_llvm_library name)
> ${install_type} DESTINATION ${install_dir}
> COMPONENT ${name})
>
> - if (NOT LLVM_ENABLE_IDE)
> + if (NOT CMAKE_CONFIGURATION_TYPES)
> add_llvm_install_targets(install-${name}
> DEPENDS ${name}
> COMPONENT ${name})
> @@ -890,7 +890,7 @@ macro(add_llvm_tool name)
> RUNTIME DESTINATION ${LLVM_TOOLS_INSTALL_DIR}
> COMPONENT ${name})
>
> - if (NOT LLVM_ENABLE_IDE)
> + if (NOT CMAKE_CONFIGURATION_TYPES)
> add_llvm_install_targets(install-${name}
> DEPENDS ${name}
> COMPONENT ${name})
> @@ -928,7 +928,7 @@ macro(add_llvm_utility name)
> install (TARGETS ${name}
> RUNTIME DESTINATION ${LLVM_UTILS_INSTALL_DIR}
> COMPONENT ${name})
> - if (NOT LLVM_ENABLE_IDE)
> + if (NOT CMAKE_CONFIGURATION_TYPES)
> add_llvm_install_targets(install-${name}
> DEPENDS ${name}
> COMPONENT ${name})
> @@ -1390,7 +1390,7 @@ function(add_lit_testsuite target commen
> endfunction()
>
> function(add_lit_testsuites project directory)
> - if (NOT LLVM_ENABLE_IDE)
> + if (NOT CMAKE_CONFIGURATION_TYPES)
> cmake_parse_arguments(ARG "" "" "PARAMS;DEPENDS;ARGS" ${ARGN})
>
> # Search recursively for test directories by assuming anything not
> @@ -1449,7 +1449,7 @@ function(llvm_install_library_symlink na
> CODE "install_symlink(${full_name} ${full_dest} ${output_dir})"
> COMPONENT ${component})
>
> - if (NOT LLVM_ENABLE_IDE AND NOT ARG_ALWAYS_GENERATE)
> + if (NOT CMAKE_CONFIGURATION_TYPES AND NOT ARG_ALWAYS_GENERATE)
> add_llvm_install_targets(install-${name}
> DEPENDS ${name} ${dest} install-${dest}
> COMPONENT ${name})
> @@ -1482,7 +1482,7 @@ function(llvm_install_symlink name dest)
> CODE "install_symlink(${full_name} ${full_dest} ${LLVM_TOOLS_INSTALL_DIR})"
> COMPONENT ${component})
>
> - if (NOT LLVM_ENABLE_IDE AND NOT ARG_ALWAYS_GENERATE)
> + if (NOT CMAKE_CONFIGURATION_TYPES AND NOT ARG_ALWAYS_GENERATE)
> add_llvm_install_targets(install-${name}
> DEPENDS ${name} ${dest} install-${dest}
> COMPONENT ${name})
>
> Modified: llvm/trunk/cmake/modules/CMakeLists.txt
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/cmake/modules/CMakeLists.txt?rev=341045&r1=341044&r2=341045&view=diff
> ==============================================================================
> --- llvm/trunk/cmake/modules/CMakeLists.txt (original)
> +++ llvm/trunk/cmake/modules/CMakeLists.txt Thu Aug 30 02:32:09 2018
> @@ -132,7 +132,7 @@ if (NOT LLVM_INSTALL_TOOLCHAIN_ONLY)
> PATTERN LLVM-Config.cmake EXCLUDE
> PATTERN GetHostTriple.cmake EXCLUDE)
>
> - if (NOT LLVM_ENABLE_IDE)
> + if (NOT CMAKE_CONFIGURATION_TYPES)
> # Add a dummy target so this can be used with LLVM_DISTRIBUTION_COMPONENTS
> add_custom_target(cmake-exports)
> add_llvm_install_targets(install-cmake-exports
>
> Modified: llvm/trunk/tools/xcode-toolchain/CMakeLists.txt
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/xcode-toolchain/CMakeLists.txt?rev=341045&r1=341044&r2=341045&view=diff
> ==============================================================================
> --- llvm/trunk/tools/xcode-toolchain/CMakeLists.txt (original)
> +++ llvm/trunk/tools/xcode-toolchain/CMakeLists.txt Thu Aug 30 02:32:09 2018
> @@ -100,7 +100,7 @@ add_llvm_install_targets(install-xcode-t
> PREFIX ${LLVMToolchainDir}/usr/)
>
> if(LLVM_DISTRIBUTION_COMPONENTS)
> - if(LLVM_ENABLE_IDE)
> + if(CMAKE_CONFIGURATION_TYPES)
> message(FATAL_ERROR "LLVM_DISTRIBUTION_COMPONENTS cannot be specified with multi-configuration generators (i.e. Xcode or Visual Studio)")
> endif()
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
>
More information about the llvm-commits
mailing list