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

Juergen,

Thanks to let me know. I guess r200762 (and r200763) might affect.

Although I won't check this on darwin box, I suspect the line in
HandleLLVMOptions.cmake;

  set(LLVM_PLUGIN_EXT ${CMAKE_SHARED_MODULE_SUFFIX})

Does it have expected value?

2014-02-13 10:11 GMT+09:00 Juergen Ributzka <juergen at
apple.com>:
> 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
>
>

No, it has the wrong value. I tried it with cmake 2.8.9 and 2.8.12.2. Both of
them set the variable to “.so”.

On Feb 12, 2014, at 5:29 PM, NAKAMURA Takumi <geek4civic at gmail.com>
wrote:
> Juergen,
> 
> Thanks to let me know. I guess r200762 (and r200763) might affect.
> 
> Although I won't check this on darwin box, I suspect the line in
> HandleLLVMOptions.cmake;
> 
>  set(LLVM_PLUGIN_EXT ${CMAKE_SHARED_MODULE_SUFFIX})
> 
> Does it have expected value?
> 
> 2014-02-13 10:11 GMT+09:00 Juergen Ributzka <juergen at apple.com>:
>> 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
>> 
>>