llvm dev - Feb 2014 - [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.

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.

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>