[cmake-developers] push of LinkOptionsCommand topic branch

Stephen Kelly steveire at gmail.com
Sat Feb 8 06:10:06 EST 2014


Steve Wilson wrote:

> Fixed.
> 
> Updated
> 
> Fixed

Great, thanks for all of that. I have force-pushed your branch, which means 
you need to do this before proceeding with further work.

1) Get remote changes, including my change to your branch.

You'll see output comparable to this, where your branch is listed as having 
a forced update:

 $ git fetch 
 remote: Counting objects: 27, done.
 remote: Compressing objects: 100% (10/10), done.
 remote: Total 11 (delta 8), reused 2 (delta 1)
 Unpacking objects: 100% (11/11), done.
 From git://cmake.org/cmake
  + 74c3875...31b4965 gcc-ipo    -> origin/gcc-ipo  (forced update)
    53cffda..d582809  master     -> origin/master
    3283439..930141d  next       -> origin/next


You can run

 gitk 74c3875...31b4965

on any of the ranges on the left to see the old and new branch in one view.

2)

 git checkout LinkOptionsCommand

3) Assuming you still have no local changes,

 git reset --hard origin/LinkOptionsCommand


That makes your local copy in sync with the remote as I have updated it. 

The reason is that I made non-fast-forward changes to your branch. For 
example, the fixup for an earlier commit should be squashed into the commit 
that introduced the need for the fixup.  I generally review commits from the 
bottom up by running `gitk origin/LinkOptionsCommand`, so seeing unchanged 
commits early with fixups later in the branch is confusing and harder to 
review. The commits need to be self-contained and reviewable now and in the 
future. Also, self-contained commits make it easier to see whether all 
relevant features in the commit have a unit test.

Additionally, commits which are incomplete, such as the `cmTarget: Make 
processCompileOptionsInternal more general.` one can not be tested properly 
by the test suite.

After checking out your branch, I ran `git rebase -i master`, moved the 
`Help:  Adjust LINK_OPTIONS property and command docs to common style.` 
commit up, and then changed the `pick` to a `f` and saved the file.

 pick 643a9c9 cmTarget: Make processCompileOptionsInternal more general.
 pick 0ead6e2 cmLocalGenerator: Add AddLinkOptions method for LINK_FLAGS.
 pick 279368f Add support for {INTERFACE_}LINK_OPTIONS.
 f e44de8a Help:  Adjust LINK_OPTIONS property and command docs to common 
style.
 pick 36f5c3b Add support for add_link_options() command.
 pick dbbfc49 Add support for target_link_options() command.
 pick 71f48a1 Export: Enable support for export()/install(EXPORT) of 
LINK_OPTIONS.
 pick c0033d7 Help: Update buildsystem documentation for 
link_options/LINK_OPTIONS.
 pick f364b08 Help: Remove extra dashes from titles in documentation.
 pick cfa9be7 Tests:  Remove tabs from add_link_options test source file.
 pick 0a4b066 Tests: Add content to target_link_options test file foo.cpp.

For more on interactive rebase see here:

 https://help.github.com/articles/interactive-rebase

Other than squashing in fixups, I've made the following changes:

1) Add a comma to {INTERFACE_,}LINK_OPTIONS in the commit message so that it 
is more like what is used in a shell.

2) Move the 

-                            std::string("Used compile ") + logName
+                            std::string("Used ") + logName

snippet from `Add support for {INTERFACE_,}LINK_OPTIONS.` to `cmTarget: Make 
processCompileOptionsInternal more general.` to complete that commit.

[git reset -p helps with this]

3) Split the STATIC_LIBRARY and OBJECT_LIBRARY differences out of 
`cmLocalGenerator: Add AddLinkOptions method for LINK_FLAGS.`

Especially changes like this need to be in a commit on their own instead of 
buried in a different commit which is aiming to do a very different thing. 
This way it can be considered/reviewed/reasoned separately with its own 
commit message.

4) Split the directory-scoped changes out of `Add support for 
{INTERFACE_,}LINK_OPTIONS.`

Commits should be indivisible, where possible and where it makes sense. 
Also, when introducing a new feature, it helps to introduce a minimal way of 
testing it first and then add all further 'porcelain' user interface in 
follow up commits. It helps to do that because the commit that introduces it 
also introduces the supporting infrastructure/tests etc and it is easier 
then to see the core of the implementation without anything extra.

The target property is the most-low level existence of this feature, so it 
makes sense to add that, together with supporting infrastructure, in the 
first commit.

5) Add a test for the LINK_OPTIONS target property.

6) Note that LINK_OPTIONS can be origin-tracked with 
CMAKE_DEBUG_TARGET_PROPERTIES.

7) Re-order the introduction of the target_link_options and add_link_options 
commands. This is only because I see the target_link_options as more 
relevant.

8) Simplify the addition of the command test. I didn't see any reason/need 
to expand the macro.

 -  add_test(CMakeCommands.target_link_options ${CMAKE_CTEST_COMMAND}
 -    --build-and-test
 -    "${CMake_SOURCE_DIR}/Tests/CMakeCommands/target_link_options"
 -    "${CMake_BINARY_DIR}/Tests/CMakeCommands/target_link_options"
 -    --build-two-config
 -    ${build_generator_args}
 -    --build-project target_link_options
 -    ${CMakeCommands.target_link_options_CTEST_OPTIONS}
 -    --build-options ${build_options}
 -    ${CMakeCommands.target_link_options_BUILD_OPTIONS}
 -  )
 -  list(APPEND TEST_BUILD_DIRS 
"${CMake_BINARY_DIR}/Tests/CMakeCommands/target_link_options")
 -
 +  ADD_TEST_MACRO(CMakeCommands.target_link_options target_link_options)


The command

 git diff 0a4b066..b8782eb

will show you the total difference I made in code.

> 
>> In the 'cmLocalGenerator: Add AddLinkOptions method for LINK_FLAGS.'
>> commit message, you mention that the differences regarding static and
>> object libraries from the xcode generator are included. You don't say
>> what impact that has on other generators though.
>> 
>> Were the other generators buggy by not doing the same thing before?
>> 
>> Or was the xcode generator special for needing this?
>> 
>> If the xcode generator has a special need, then that snippet should stay
>> in the xcode generator, right?
> 
> 
> Good questions.   The other generators did not specifically (that I
> noticed) have checks for static/object libraries.   It seems to me though
> that they maybe should have which is why I put the checks in
> AddLinkOptions.

Right. Even if it should indeed be changed, it belongs in a separate commit 
anyway so that it can be reviewed in isolation too.

Thanks,

Steve.






More information about the cmake-developers mailing list