Stephen Kelly
2014-Feb-10 09:54 UTC
[LLVMdev] [llvm] r201072 - [CMake] Introduce llvm_add_library().
NAKAMURA Takumi wrote:> [CMake] Introduce llvm_add_library().I recommend moving away from wrappers like this. They indicate that either CMake is not providing the interfaces needed, or not propagating them, or that they exist but are not used. Such wrappers don't parse the arguments in the same way as the wrapped command etc. Wrappers are not good API proxies. Additionally, you put people who know cmake already at a disadvantage because you make the code look like 'not cmake code' by using llvm_add_library instead of add_library, the cmake command. Making that wrapper macro also invoke target_link_libraries looks odd to someone familiar with cmake. I don't fully know what this wrapper is needed for, but I looked at it a bit. if(ARG_MODULE) set_property(TARGET ${name} PROPERTY SUFFIX ${LLVM_PLUGIN_EXT}) endif() if(ARG_SHARED) if (MSVC) set_target_properties(${name} PROPERTIES IMPORT_SUFFIX ".imp") endif () endif() CMake provides CMAKE_SHARED_MODULE_SUFFIX and CMAKE_IMPORT_LIBRARY_SUFFIX. Can you set those at directory scope instead of setting the target property in the macro? Anyway, just a note to consider moving instead in a direction of fewer wrappers, rather than more. I'm not familiar enough with the llvm buildsystem to know why you're doing some of the things you're doing, but if you tell us about the gaps in the cmake offering we can try to fill them. Thanks, Steve.
NAKAMURA Takumi
2014-Feb-10 10:34 UTC
[LLVMdev] [llvm] r201072 - [CMake] Introduce llvm_add_library().
Steve, excuse me to respond you partially. 2014-02-10 18:54 GMT+09:00 Stephen Kelly <steveire at gmail.com>:> NAKAMURA Takumi wrote: > >> [CMake] Introduce llvm_add_library(). > > I recommend moving away from wrappers like this. They indicate that either > CMake is not providing the interfaces needed, or not propagating them, or > that they exist but are not used. > > Such wrappers don't parse the arguments in the same way as the wrapped > command etc. Wrappers are not good API proxies. Additionally, you put people > who know cmake already at a disadvantage because you make the code look like > 'not cmake code' by using llvm_add_library instead of add_library, the cmake > command. Making that wrapper macro also invoke target_link_libraries looks > odd to someone familiar with cmake. > > I don't fully know what this wrapper is needed for, but I looked at it a > bit. > > if(ARG_MODULE) > set_property(TARGET ${name} PROPERTY SUFFIX ${LLVM_PLUGIN_EXT}) > endif() > > if(ARG_SHARED) > if (MSVC) > set_target_properties(${name} > PROPERTIES > IMPORT_SUFFIX ".imp") > endif () > endif() > > > CMake provides CMAKE_SHARED_MODULE_SUFFIX and CMAKE_IMPORT_LIBRARY_SUFFIX. > Can you set those at directory scope instead of setting the target property > in the macro?Reasonable. They could be put into common configurator, HandleLLVMOptions. For now in r201072, I simply gather common logics around there. I'd be happy if llvm_add_library became simpler, and I will work on.> Anyway, just a note to consider moving instead in a direction of fewer > wrappers, rather than more. I'm not familiar enough with the llvm > buildsystem to know why you're doing some of the things you're doing, but if > you tell us about the gaps in the cmake offering we can try to fill them. > > Thanks, > > Steve.
Juergen Ributzka
2014-Feb-13 01:11 UTC
[LLVMdev] [llvm] r201072 - [CMake] Introduce llvm_add_library().
Hi Takumi, I am not sure if it this change, but recently we started to build LLVMHello.so and BugpointPasses.so on OS X. A few bugpoint tests are failing, because they are looking for a dylib that doesn’t exist. Could you please take a look? Thanks -Juergen On Feb 10, 2014, at 2:34 AM, NAKAMURA Takumi <geek4civic at gmail.com> wrote:> Steve, excuse me to respond you partially. > > 2014-02-10 18:54 GMT+09:00 Stephen Kelly <steveire at gmail.com>: >> NAKAMURA Takumi wrote: >> >>> [CMake] Introduce llvm_add_library(). >> >> I recommend moving away from wrappers like this. They indicate that either >> CMake is not providing the interfaces needed, or not propagating them, or >> that they exist but are not used. >> >> Such wrappers don't parse the arguments in the same way as the wrapped >> command etc. Wrappers are not good API proxies. Additionally, you put people >> who know cmake already at a disadvantage because you make the code look like >> 'not cmake code' by using llvm_add_library instead of add_library, the cmake >> command. Making that wrapper macro also invoke target_link_libraries looks >> odd to someone familiar with cmake. >> >> I don't fully know what this wrapper is needed for, but I looked at it a >> bit. >> >> if(ARG_MODULE) >> set_property(TARGET ${name} PROPERTY SUFFIX ${LLVM_PLUGIN_EXT}) >> endif() >> >> if(ARG_SHARED) >> if (MSVC) >> set_target_properties(${name} >> PROPERTIES >> IMPORT_SUFFIX ".imp") >> endif () >> endif() >> >> >> CMake provides CMAKE_SHARED_MODULE_SUFFIX and CMAKE_IMPORT_LIBRARY_SUFFIX. >> Can you set those at directory scope instead of setting the target property >> in the macro? > > Reasonable. They could be put into common configurator, HandleLLVMOptions. > For now in r201072, I simply gather common logics around there. > I'd be happy if llvm_add_library became simpler, and I will work on. > >> Anyway, just a note to consider moving instead in a direction of fewer >> wrappers, rather than more. I'm not familiar enough with the llvm >> buildsystem to know why you're doing some of the things you're doing, but if >> you tell us about the gaps in the cmake offering we can try to fill them. >> >> Thanks, >> >> Steve. > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140212/37485031/attachment.html>