[cmake-developers] push of LinkOptionsCommand topic branch

[Stephen Kelly]

Steve Wilson wrote:

> I have applied all the suggestions and re-pushed LinkOptionsCommand (after
> rebasing) to stage.

Thanks for that.

1) Your first commit should be split up into at least two commits. 

The first commit in your topic should probably refactor the generators to 
use a new cmLocalGenerator::AddLinkOptions method which uses the LINK_FLAGS. 
See eg commit 35496761 where I did similar for COMPILE_FLAGS. The 
AddLinkOptions will have the special handling of OBJECT_LIBRARY and 
STATIC_LIBRARY from the Xcode generator. If that is appropriate for all 
generators, the commit message should say why.

The second commit should add the COMPILE_OPTIONS handling to that method.

My request that you create commits which change the user interface along 
with all supporting code to do so may have been confusing. Really what is 
needed is to create commits which are complete, self-contained and doing one 
thing at a time. That's why it makes sense to split the first commit up into 
two parts. If it makes sense to split it into further self-contained and 
complete parts, feel free to do so. I just wanted to discourage commits 
which are divided up by 'changes to files' rather than 'conceptual changes'. 

For example, changing  processCompileOptionsInternal to 
processOptionsInternal and changing the logName at call sites could be a 
standalone commit preceeding your first commit.

2) The change to cmNinjaNormalTargetGenerator seems incorrect. Should 
linkFlags should be used with AddLinkOptions?

3) Documentation title underlines should be as long as the title, not 3 
dashes longer.

4) Tests should avoid bad practices like using target_link_options to add 
libraries. Instead try to choose suitable link flags for the tests. 

On GNU, you can do this:

 add_executable(foo foo.cpp)
 target_link_options(foo PRIVATE -Wl,--ignore-unresolved-symbol,main)

and write a foo.cpp which does not define main.

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). 

The documentation should possibly be clear that the contents of LINK_OPTIONS 
should be suitable for passing to the driver, not directly to the linker.

6) For the ExportImport test, you should create some export target on the 
Export side, and use it on the Import side. For example, on the Export side, 
do something like 

 add_library(no_main_is_ok INTERFACE)
 target_link_options(no_main_is_ok 
   INTERFACE -Wl,--ignore-unresolved-symbol,main)
 # ... Export the target.

and on the Import side, 

 add_executable(exe_no_main exe_no_main.cpp)
 target_link_libraries(exe_no_main no_main_is_ok)

That should be done for both the import from the build tree and the install 
tree, as much of the existing code in Import/ does.

7) I've added two commits to your branch. Please squash them into the 
appropriate commits in your topic.

Thanks,

Steve.


> Thanks,
> 
> SteveW
> 
> On Feb 2, 2014, at 2:44 AM, Stephen Kelly
> <steveire at gmail.com> wrote:
> 
>> Steve Wilson wrote:
>> 
>>> I have just pushed the LinkOptionsCommand topic branch to stage.   This
>>> topic branch contains commits that implement support for:
>>> 
>>> add_link_options()
>>> 
>>> target_link_options()
>>> 
>>> and the LINK_OPTIONS variable.
>>> 
>>> Please take a look as interested and send me feedback.
>> 
>> 
>> Thanks for that.
>> 
>> 1) The first thing I notice is that I don't think you've broken the
>> commits up along the correct fault lines. It would make more sense to
>> squash the changes to cmTarget, cmLocalGenerator, the generators, the
>> DAGChecker and the cmTargetLinkOptionsCommand together along with the
>> help for that command and target properties and the tests for it.
>> Actually, instead of squashing it into one commit, you can consider
>> creating multiple commits, eg one which adds the
>> {INTERFACE_,}LINK_OPTIONS props (and tests and documents them), and
>> another which adds the command (with tests and docs). Consider how you
>> can split your commits along 'interface changes' 'fault lines'.
>> 
>> In follow-up commits you can add the cmAddLinkOptionsCommand with the
>> changes to the cmMakefile (and tests and docs), and exporting (with test
>> and docs extensions).
>> 
>> That order of commits makes it clear what the dependent changes are. The
>> cmAddLinkOptionsCommand and changes to the cmMakefile are convenience
>> only. The relevant change to CMake is support for the target property,
>> and the high-level way to set that target property is
>> cmTargetLinkOptionsCommand.
>> 
>> As it is, your first commit adds changes to the cmMakefile interface but
>> no users of the new AddLinkOption interface until the command is added.
>> That's why that change should be in the same commit as the new command.
>> 
>> The commit message would then describe changes relevant to the user
>> interface, instead of only specific changes to the class interfaces,
>> which probably don't belong in the commit message anyway.
>> 
>> If you don't know how to rebase with git, just leave all of this for now.
>> 
>> 2) The processLinkOptionsInternal method is almost identical to
>> processCompileOptionsInternal. Consider renaming the latter (in a
>> separate, standalone commit), refactoring it and using it instead.
>> 
>> 3) Your topic doesn't remove code from the generators which reads the
>> LINK_FLAGS target property. As the new cmLocalGenerator method will do
>> that, you can remove it from the concrete generators. See eg commit
>> 35496761 where I did similar for COMPILE_FLAGS.
>> 
>> 4) The use of PROCESS_BEFORE in cmTargetCompileOptionsCommand seems to be
>> a bug, don't repeat it in cmTargetLinkOptionsCommand.
>> 
>> 5) The commit which adds exporting of the new target property should
>> extend the ExportImport unit test.
>> 
>> 6) New docs should link to target properties with rst code like
>> :prop_tgt:`LINK_OPTIONS`. Some of the COMPILE_OPTIONS related doc is too
>> :old
>> to link like this properly, so see Help/manual/cmake-buildsystem.7.rst
>> for example.
>> 
>> 7) Consider extending Help/manual/cmake-buildsystem.7.rst with notes
>> about setting the LINK_OPTIONS and new commands.
>> 
>> 8) One of the reasons I didn't add LINK_OPTIONS before was that I didn't
>> know whether it should accept "--no-undefined" or "-Wl,--no-undefined",
>> or both. Is the latter compiler-driver-specific? Is that irrelevant
>> because the link option is tool specific anyway?
>> 
>> 9) Use spaces, not tabs, even in tests.
>> 
>> I've listed a lot of feedback, but it's all minor stuff. Thanks for the
>> contribution.
>> 
>> 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