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