[llvm] r341045 - Revert "[CMake] Use LLVM_ENABLE_IDE instead of CMAKE_CONFIGURATION_TYPES"

Tue Oct 9 12:43:33 PDT 2018

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
>
>