[flang] Refine how Clang dependencies are expressed #58663

[Da-Viper]

Summary

Fixes #58663

[Da-Viper]

Hello There,

This is my first contribution and I want to make sure I am doing it the right way.

This is to make the code reusable and easy to read.

I am going to

• convert the macro add_flang_library to a function as I do not see any reason it is a macro (Please correct me if I am wrong)
• explicitly name the cmake_parse_agruments arguments
• do the same as the above for flang/FrontendTool for other flang directories

Possibly extend this to the lldb part too.

Is this the case ?

Thanks

[awarzynski]

Hello @Da-Viper , thanks for sending this!

Would you be able to upload a diff with more context? See https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface. You may find it much easier to use Arcanist for all your interactions with Phabricator ;-)

flang/cmake/modules/AddFlang.cmake

66

Is this macro capable of differentiating between DYLIB and non-DYLIB builds?

flang/lib/FrontendTool/CMakeLists.txt

21

Unrelated change

[Da-Viper]

Changed the name of the flang_add_library to reflect it meaning
apply changes to other files

[Da-Viper]

In D140998#4028291, @awarzynski wrote:

Hello @Da-Viper , thanks for sending this!

Would you be able to upload a diff with more context? See https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface. You may find it much easier to use Arcanist for all your interactions with Phabricator ;-)

I think I did it the right way take a look

flang/cmake/modules/AddFlang.cmake

66

It is not, It is only for dynamic libs I would update the name to reflect that

66

Looking at previous codes, if you are linking statically it is only clang-cpp that is included else it is anything that is added to the option CLANG_DYLIBS

the rest is then handled by clang_target_link_libraries see AddClang.cmake#L209

[awarzynski]

Sorry, I was meant to reply earlier!

In D140998#4026425, @Da-Viper wrote:

This is my first contribution and I want to make sure I am doing it the right way.

You are - thanks for contributing :)

This is to make the code reusable and easy to read.

Sounds good! One other goal worth considering - alignment with other sub-projects.

I am going to

• convert the macro add_flang_library to a function as I do not see any reason it is a macro (Please correct me if I am wrong)
• explicitly name the cmake_parse_agruments arguments
• do the same as the above for flang/FrontendTool for other flang directories

+1

Possibly extend this to the lldb part too.

Why not do it for all sub-projects? That would be good for consistency. I don't really contribute to other projects - you may want to ask people on Discourse first: https://discourse.llvm.org/.

These changes makes sense- thank you! From my experience, it's good to thoroughly test any CMake changes locally before merging upstream. What CMake configurations have you tested? Basically, you want to make sure that Flang buildbots (https://lab.llvm.org/buildbot/#/builders <--- type "flang" in search box) remain in good shape.

flang/lib/Frontend/CMakeLists.txt

55 ↗

(On Diff #486534)

Why CLANG_DYLIBS? In LLDB they would use CLANG_LIBS: https://github.com/llvm/llvm-project/blob/beb997799d823aece097494f156c6d277d26571c/lldb/cmake/modules/AddLLDB.cmake#L42

[Da-Viper]

Updating D140998: [flang] Refine how Clang dependencies are expressed #58663

rename CLANG_DYLIBS to CLANG_LIBS

[Da-Viper]

These changes makes sense- thank you! From my experience, it's good to thoroughly test any CMake changes locally before merging upstream. What CMake configurations have you tested? Basically, you want to make sure that Flang buildbots (https://lab.llvm.org/buildbot/#/builders <--- type "flang" in search box) remain in good shape.

I am testing it with the LLVM_BUILD_LLVM_DYLIB option on and off but it takes more than an hour to build on my system

flang/lib/Frontend/CMakeLists.txt

55 ↗

(On Diff #486534)

I confused one for the other, reverted it

[awarzynski]

In D140998#4031125, @Da-Viper wrote:

I am testing it with the LLVM_BUILD_LLVM_DYLIB option on and off but it takes more than an hour to build on my system

That's probably the main flag to test, so we can be fairly confident that this is correct. And yes, Flang build times are very long :(

LGTM, but please address my comment in flang/lib/FrontendTool/CMakeLists.txt first. Do you have commit access for llvm-project?

[Da-Viper]

flang/lib/FrontendTool/CMakeLists.txt

21

This is now handled by clang_target_link_libraries function that is called in the add_flang_library function as it does the same thing

flang/cmake/modules/AddFlang.cmake

   cmake_parse_arguments(ARG
     "SHARED;STATIC;INSTALL_WITH_TOOLCHAIN"
     ""
-    "ADDITIONAL_HEADERS"
+    "ADDITIONAL_HEADERS;CLANG_LIBS"
     ${ARGN})
   set(srcs)
   if (MSVC_IDE OR XCODE)

 
   llvm_add_library(${name} ${LIBTYPE} ${ARG_UNPARSED_ARGUMENTS} ${srcs})
 
+  clang_target_link_libraries(${name} PRIVATE ${ARG_CLANG_LIBS})
+
   if (TARGET ${name})
 
     if (NOT LLVM_INSTALL_TOOLCHAIN_ONLY OR ${name} STREQUAL "libflang"

flang/lib/FrontendTool/CMakeLists.txt

   LINK_COMPONENTS
   Option
   Support
-)
 
-if(CLANG_LINK_CLANG_DYLIB)
-  add_dependencies(flangFrontend clang-cpp)
-else()
-  add_dependencies(flangFrontendTool clangBasic)
-endif()
-
-clang_target_link_libraries(flangFrontendTool
-  PRIVATE
+  CLANG_LIBS
   clangBasic
   clangDriver
-)
+)
 No newline at end of file