Steve Wilson wrote:

> Let’s try all this again.    I have a couple questions.
> 
> On Feb 4, 2014, at 3:41 AM, Stephen Kelly
> <steve...@gmail.com> wrote:
> 
>> 2) The change to cmNinjaNormalTargetGenerator seems incorrect. Should
>> linkFlags should be used with AddLinkOptions?
> 
> Do you mean linkFlags vs vars[“LINK_FLAGS”]?  

Yes, that's what I meant. The linkFlags variable is populated, but not used.

I may be mis-reading the code. I'll have a closer look tomorrow.

>> 3) Documentation title underlines should be as long as the title, not 3
>> dashes longer.
> 
> Ok great.   What specifically are you referring to with this comment?

 diff --git a/Help/command/add_link_options.rst 
b/Help/command/add_link_options.rst
 index 60a981d..a4e2955 100644
 --- a/Help/command/add_link_options.rst                                        
                                                                                
                                                    
 +++ b/Help/command/add_link_options.rst                                        
                                                                                
                                                    
 @@ -1,5 +1,5 @@                                                                
                                                                                
                                                    
  add_link_options                                                              
                                                                                
                                                    
 --------------------                                                           
                                                                                
                                                    
 +----------------                                                              
                                                                                
                                                    
                                                                                
                                                                                
                                                   
  Adds options to the link commands of targets.                                 
                                                                                
                                                    
                 

Repeats for other docs and titles.

> 
>> 4) Tests should avoid bad practices like using target_link_options to add
>> libraries. Instead try to choose suitable link flags for the tests.
> 
> I’m having a little trouble with your notion of ‘bad practices.’   I’m
> sure you have good reasons for making the determination that they are bad
> practices, but to me they seem somewhat arbitrary.

Generally, cmake managed the libraries that are linked to (specified via 
target_link_libraries) so that it can determine the proper link line, and 
set up dependencies in the buildsystem so that a change in the library 
causes a rebuild. Specifying a library in target_link_options would bypass 
that. I don't know of a reason that 

 target_link_options(tgt PRIVATE -llibsomething)

is better than

 target_link_libraries(tgt PRIVATE -llibsomething)

and should be encouraged. If there is a reason, maybe I just don't know 
about it.

> Is there some design document that you are using to make these
> determinations?

No, just awareness of the cmake feature and existing commands etc for 
handling libraries, and some awareness of the reason to use the particular 
commands. I would say that using target_link_options() to link to libraries 
should be discouraged in the documentation of that command unless there's a 
particular reason to recommend it in some case?

> I realize that CMake has different mechanisms for explicitly
> handling libraries, but the point isn’t to handle the library, the point
> was just to pass a reasonable linker option.

Ok, I'm not aware of a reason to do that, but if there is a good one, it 
might be worth adding it to the documentation.

>> On GNU, you can do this:
>> 
>> add_executable(foo foo.cpp)
>> target_link_options(foo PRIVATE -Wl,--ignore-unresolved-symbol,main)

> Does that make sense?  How should I determine what is bad practice in the
> face of what is reasonable?

I would say that tests and documentation should assume the use of the CMake 
features for managing the linked libraries. 

Additionally, I find the test with the --ignore-unresolved-symbol option 
preferable because it does not rely on the existence of outside files.

>> 5) Regarding what I said before about accepting -Wl,--no-undefined versus
>> accepting --no-undefined, I think the case of flags with arguments as
>> above shows that only the -Wl, prefixed case should be accepted (as your
>> branch already does).
> 
> However, what if someone changes the mechanism that picks the linker so
> that instead of using the compiler
> driver to drive the link stage, they use the actual linker?   I don’t do
> such a thing with my build systems and
> in all probability the majority of users would never need to make such a
> change.   However, if I, for some reason, *did* need to change CMake so
> that the link stage invoked the linker directly, I would absolutely expect
> the link options commands to pass whatever linker option I deemed
> necessary to the linker.

Indeed. Brad, any input here? Is this even possible?

Thanks,

Steve.


-- 

Powered by www.kitware.com

Visit other Kitware open-source projects at 
http://www.kitware.com/opensource/opensource.html

Please keep messages on-topic and check the CMake FAQ at: 
http://www.cmake.org/Wiki/CMake_FAQ

Follow this link to subscribe/unsubscribe:
http://public.kitware.com/cgi-bin/mailman/listinfo/cmake-developers

Reply via email to