Re: [cmake-developers] push of LinkOptionsCommand topic branch
Hi Steve(s), On 02/12/2014 12:06 PM, Stephen Kelly wrote: Steve Wilson wrote: I saved a copy of the branch in another of the repository. The commit numbers didn’t change and as far as I can tell they are still in the same order that I had them in when I initially pushed the branch.There are no rebases removing the downstream updates, etc... Ah, the problem might be that your remote name is not origin, so the reset did nothing. Find the correct remote and branch. Run gitk to see it. The committer on the commits should be me. Did the reset --hard output an error message tell you what was going on? What is the status of this topic currently? It adds a great feature! Thanks, -Brad -- 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
Re: [cmake-developers] push of LinkOptionsCommand topic branch
Brad King wrote:
What is the status of this topic currently? It adds a great feature!
I've been waiting for master to re-open for features before working on it.
Here are some open issues:
1) INTERFACE_LINK_OPTIONS presents a new issue that was not present before
with the LINK_FLAGS. If I understand correctly, if I set LINK_FLAGS on a
static library, the contents are passed to the archiver (CMAKE_AR), whereas
if I set it on a shared library or executable the contents are passed to the
linker (CMAKE_LINKER). So, we can't have code as simple as:
add_library(foo STATIC foo.cpp)
target_link_options(foo INTERFACE --no-undefined)
but require:
add_library(foo STATIC foo.cpp)
set (_exe $STREQUAL:$TARGET_PROPERTY:EXECUTABLE)
set (_shlib $STREQUAL:$TARGET_PROPERTY:EXECUTABLE)
target_link_options(foo INTERFACE
$$OR:${_exe},${_shlib}:--no-undefined
)
Consider splitting the use-cases and adding both {INTERFACE_,}LINK_OPTIONS
and {INTERFACE_,}ARCHIVE_OPTIONS instead.
2) Should linker options or compiler driver options be specified in the
LINK_OPTIONS? That is
--no-undefined
or
-Wl,--no-undefined
My preference currently is the former, and have CMake do transformation if
passing options to the compiler driver, assuming cmake can tell the
difference between that and an actual linker.
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
Re: [cmake-developers] push of LinkOptionsCommand topic branch
On 02/19/2014 11:00 AM, Stephen Kelly wrote:
I've been waiting for master to re-open for features before working on it.
It is now open for post-3.0 development :)
1) INTERFACE_LINK_OPTIONS presents a new issue that was not present before
with the LINK_FLAGS. If I understand correctly, if I set LINK_FLAGS on a
static library, the contents are passed to the archiver (CMAKE_AR), whereas
if I set it on a shared library or executable the contents are passed to the
linker (CMAKE_LINKER).
I don't think LINK_FLAGS are used for the archiver or librarian.
We have a separate STATIC_LIBRARY_FLAGS for that:
http://www.cmake.org/cmake/help/v2.8.12/cmake.html#prop_tgt:STATIC_LIBRARY_FLAGS
Consider splitting the use-cases and adding both {INTERFACE_,}LINK_OPTIONS
and {INTERFACE_,}ARCHIVE_OPTIONS instead.
Yes. That will be consistent with LINK_FLAGS/STATIC_LIBRARY_FLAGS
but with better naming.
2) Should linker options or compiler driver options be specified in the
LINK_OPTIONS? That is
--no-undefined
or
-Wl,--no-undefined
My preference currently is the former, and have CMake do transformation if
passing options to the compiler driver, assuming cmake can tell the
difference between that and an actual linker.
Yes, that makes sense. It may take some work in the platform info
file rule variables so that the generators can tell when they are
invoking the linker directly v. through a compiler, and what the
wrapper option (-Wl,) for the compiler should be.
Thanks,
-Brad
--
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
Re: [cmake-developers] push of LinkOptionsCommand topic branch
Brad King wrote:
On 02/19/2014 11:00 AM, Stephen Kelly wrote:
I've been waiting for master to re-open for features before working on
it.
It is now open for post-3.0 development :)
Yep, great. Much better than waiting for weeks during RC phase. :)
Consider splitting the use-cases and adding both
{INTERFACE_,}LINK_OPTIONS and {INTERFACE_,}ARCHIVE_OPTIONS instead.
Yes. That will be consistent with LINK_FLAGS/STATIC_LIBRARY_FLAGS
but with better naming.
My preference currently is the former, and have CMake do transformation
if passing options to the compiler driver, assuming cmake can tell the
difference between that and an actual linker.
Yes, that makes sense. It may take some work in the platform info
file rule variables so that the generators can tell when they are
invoking the linker directly v. through a compiler, and what the
wrapper option (-Wl,) for the compiler should be.
Those are the only two issues I know of. This topic isn't a high priority
for me though. I'll be working on other topics first.
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
Re: [cmake-developers] push of LinkOptionsCommand topic branch
I’m happy to keep working on the topic as well. I just have to let it bubble
back up to the top of my queue.
SteveW
On Feb 19, 2014, at 9:27 AM, Stephen Kelly steve...@gmail.com wrote:
Brad King wrote:
On 02/19/2014 11:00 AM, Stephen Kelly wrote:
I've been waiting for master to re-open for features before working on
it.
It is now open for post-3.0 development :)
Yep, great. Much better than waiting for weeks during RC phase. :)
Consider splitting the use-cases and adding both
{INTERFACE_,}LINK_OPTIONS and {INTERFACE_,}ARCHIVE_OPTIONS instead.
Yes. That will be consistent with LINK_FLAGS/STATIC_LIBRARY_FLAGS
but with better naming.
My preference currently is the former, and have CMake do transformation
if passing options to the compiler driver, assuming cmake can tell the
difference between that and an actual linker.
Yes, that makes sense. It may take some work in the platform info
file rule variables so that the generators can tell when they are
invoking the linker directly v. through a compiler, and what the
wrapper option (-Wl,) for the compiler should be.
Those are the only two issues I know of. This topic isn't a high priority
for me though. I'll be working on other topics first.
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
signature.asc
Description: Message signed with OpenPGP using GPGMail
--
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
Re: [cmake-developers] push of LinkOptionsCommand topic branch
Steve Wilson wrote: when I do the checkout followed by the reset —hard, all I get is the same set of commits that I had before. What makes you conclude they are the same? 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
Re: [cmake-developers] push of LinkOptionsCommand topic branch
I saved a copy of the branch in another of the repository. The commit numbers didn’t change and as far as I can tell they are still in the same order that I had them in when I initially pushed the branch.There are no rebases removing the downstream updates, etc... On Feb 12, 2014, at 2:08 AM, Stephen Kelly steve...@gmail.com wrote: Steve Wilson wrote: when I do the checkout followed by the reset —hard, all I get is the same set of commits that I had before. What makes you conclude they are the same? 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 signature.asc Description: Message signed with OpenPGP using GPGMail -- 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
Re: [cmake-developers] push of LinkOptionsCommand topic branch
Steve Wilson wrote: I saved a copy of the branch in another of the repository. The commit numbers didn’t change and as far as I can tell they are still in the same order that I had them in when I initially pushed the branch.There are no rebases removing the downstream updates, etc... Ah, the problem might be that your remote name is not origin, so the reset did nothing. Find the correct remote and branch. Run gitk to see it. The committer on the commits should be me. Did the reset --hard output an error message tell you what was going on? 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
Re: [cmake-developers] push of LinkOptionsCommand topic branch
On Feb 8, 2014, at 4:10 AM, Stephen Kelly steve...@gmail.com wrote: 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 The command git diff 0a4b066..b8782eb My apologies for the delay. I have been side tracked in some other work. Perhaps I’m misunderstanding these directions but when I fetch the updates from stage, I don’t get any modified commits.I got the (forced update) message, but when I do the checkout followed by the reset —hard, all I get is the same set of commits that I had before. signature.asc Description: Message signed with OpenPGP using GPGMail -- 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
Re: [cmake-developers] push of LinkOptionsCommand topic branch
On 2014-02-08 06:10, Stephen Kelly wrote: 3) Assuming you still have no local changes, git reset --hard origin/LinkOptionsCommand Worth adding: If you *do* have local changes, you can (before running the above) set them aside with git stash and (after running the above) restore them with git stash pop. At worst, the forced push will have changed things so much that your changes can no longer be applied. If this happens, your stash will not be discarded and can be used to see your changes in their original context (and will also prevent your original version of the branch from being garbage-collected from your local clone for as long as the stash exists). Note also that reset --hard will discard any uncommitted changes! -- Matthew -- 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
Re: [cmake-developers] push of LinkOptionsCommand topic branch
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
-
Re: [cmake-developers] push of LinkOptionsCommand topic branch
Steve Wilson wrote: On Feb 6, 2014, at 3:56 PM, Stephen Kelly steve...@gmail.com wrote: There are a few things I'd like to touch up a bit. How comfortable are you with git? Would it cause problems for you if I force push your branch, or would you know how to handle that? Do you have further local changes? I’m a relative git newbie. I can get around ok and am learning a bunch as I go. The term ‘force push’ isn’t familiar though so I’m afraid you’ll have to explain (or I can look it up). I do not have any more local changes. I’ve switched to working on a different feature. Ok, I'll not force push it yet but will do that later and tell you what you need to do in reaction then. You still have extra dashes in the titles in the target property documentation. Please also rebase to master. The documentation has been updated to add more relevant links, markup etc. Please follow the same patterns in all the new docs on your branch. Note also that your add_link_options doc copied some content from add_compile_commands without modifying it (re include directories). Here's some guidance Brad gave me a while ago regarding writing commit messages with an imperative mood: http://thread.gmane.org/gmane.comp.programming.tools.cmake.devel/6904 The new tests look good to me. Please use spaces not tabs in foo.cpp in the add_link_options test. You also add a foo.cpp in the target_link_options test, but it has no content. Is that deliberate, or should it be the same as the other? 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? 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
Re: [cmake-developers] push of LinkOptionsCommand topic branch
On Feb 7, 2014, at 1:45 AM, Stephen Kelly steve...@gmail.com wrote: You still have extra dashes in the titles in the target property documentation. Fixed. Please also rebase to master. The documentation has been updated to add more relevant links, markup etc. Please follow the same patterns in all the new docs on your branch. Updated Note also that your add_link_options doc copied some content from add_compile_commands without modifying it (re include directories). Fixed Here's some guidance Brad gave me a while ago regarding writing commit messages with an imperative mood: http://thread.gmane.org/gmane.comp.programming.tools.cmake.devel/6904 Thanks, these tips are quite helpful. The new tests look good to me. Please use spaces not tabs in foo.cpp in the add_link_options test. You also add a foo.cpp in the target_link_options test, but it has no content. Is that deliberate, or should it be the same as the other? In theory it doesn’t matter if foo.cpp is empty for the target_link_options test. I went ahead and added some content though to match the add_link_options test, just to be consistent. 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. If this decision should be changed though, I have no problem changing it. I’m no expert on the generators. I pushed the other changes back to stage. signature.asc Description: Message signed with OpenPGP using GPGMail -- 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
Re: [cmake-developers] push of LinkOptionsCommand topic branch
I have implemented all the suggestions from the list below (modulo number 5 which needs more input from others). I have pushed the new branch to stage. SteveW On Feb 4, 2014, at 3:41 AM, Stephen Kelly steve...@gmail.com wrote: 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. signature.asc Description: Message signed with OpenPGP using GPGMail -- 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
Re: [cmake-developers] push of LinkOptionsCommand topic branch
Steve Wilson wrote: I have implemented all the suggestions from the list below (modulo number 5 which needs more input from others). I have pushed the new branch to stage. Great, thanks! I looked through it and the division among the commits looks good to me now. There are a few things I'd like to touch up a bit. How comfortable are you with git? Would it cause problems for you if I force push your branch, or would you know how to handle that? Do you have further local changes? 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
Re: [cmake-developers] push of LinkOptionsCommand topic branch
On Feb 6, 2014, at 3:56 PM, Stephen Kelly steve...@gmail.com wrote: There are a few things I'd like to touch up a bit. How comfortable are you with git? Would it cause problems for you if I force push your branch, or would you know how to handle that? Do you have further local changes? I’m a relative git newbie. I can get around ok and am learning a bunch as I go. The term ‘force push’ isn’t familiar though so I’m afraid you’ll have to explain (or I can look it up). I do not have any more local changes. I’ve switched to working on a different feature. signature.asc Description: Message signed with OpenPGP using GPGMail -- 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
Re: [cmake-developers] push of LinkOptionsCommand topic branch
Steve Wilson wrote: Ok, I’ve pushed the new changes. Are you sure? Looking at the top commit, I don't see any change to the ExportImport test. 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
Re: [cmake-developers] push of LinkOptionsCommand topic branch
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”]? I suppose it could use linkFlags. I could just revert the call to GetTargetFlags to use vars[“LINK_FLAGS”]. I was trying to avoid the case of getting the link flags for the target twice. Ie getting them once in vars[“LINK_FLAGS”] and then again in AddLinkOptions(). 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? 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. Is there some design document that you are using to make these determinations? I’m not trying to cause problems, I’m just saying that adding a library as a linker flag is a perfectly normal/legitimate thing to do coming from a Makefile world (or other build environment world). 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. 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. I agree this would make a good test, but it doesn’t change my earlier comment. Adding a library via a linker option is a perfectly valid use of linker options. If it wasn’t, the linker (or the compiler driver) wouldn’t support it. Does that make sense? How should I determine what is bad practice in the face of what is reasonable? 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. I don’t think that this suggestion is the right way to go. I will of course defer to your judgement, but I don’t agree. Perhaps there is a detail I’m missing about how the code makes a distinction between —no-undefined and -Wl,—no-undefined. 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. signature.asc Description: Message signed with OpenPGP using GPGMail -- 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
Re: [cmake-developers] push of LinkOptionsCommand topic branch
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
Re: [cmake-developers] push of LinkOptionsCommand topic branch
On Feb 5, 2014, at 3:06 PM, Stephen Kelly steve...@gmail.com wrote: Steve Wilson wrote: Now, everything you have said about not encouraging this kind of usage for target_link_options() and libraries, etc… is valid. However, does that standard apply to tests. Are tests just tests? Admittedly, the target_compile_options tests use defines as test input. I'd gladly swap that out for an alternative flag if there were a suitable flag which gave us the same test coverage on the dashboard. The add_compile_options command documents itself as not suitable for preprocessor defines and include directories, however. I guess target_compile_options documentation should get a similar note. I would also like to see the target_link_options documentation discourage use for specifying libraries. If you feel so strongly about using a -llibrary flag in the tests, then that's ok, but for me 'the file must exist' is a winning argument in favor of not doing that. I agree, ‘the file must exist’ is a winning argument. I’m not trying to push for this type of test of using libraries with target_link_options or add_link_options. (I’m already working on changes on the order that you suggested). My question has evolved more into the question of ‘what are first principles for tests?' signature.asc Description: Message signed with OpenPGP using GPGMail -- 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
Re: [cmake-developers] push of LinkOptionsCommand topic branch
Steve Wilson wrote: I’m not trying to push for this type of test of using libraries with target_link_options or add_link_options. (I’m already working on changes on the order that you suggested). My question has evolved more into the question of ‘what are first principles for tests?' I don't think the cmake community has an enforced answer to that question. For me personally, I prefer to avoid practices which I would discourage in documentation. It is not unusual for me to look at the tests of a project which I'm trying to understand better, and generally tests which are doing things which shouldn't be done are marked as such and are outnumbered by tests which do things which should be done. So, that's what I'd encourage and why, but it's not a hard requirement for CMake contributions. 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
Re: [cmake-developers] push of LinkOptionsCommand topic branch
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
steve...@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
Re: [cmake-developers] push of LinkOptionsCommand topic branch
Stephen Kelly wrote: 7) I've added two commits to your branch. Please squash them into the appropriate commits in your topic. Oh, forgot this one: 8) I get some unit test failures: The following tests FAILED: 85 - LinkFlags-dll_config (Failed) 87 - LinkFlags-exe_config (Failed) 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
Re: [cmake-developers] push of LinkOptionsCommand topic branch
On Feb 4, 2014, at 3:49 AM, Stephen Kelly steve...@gmail.com wrote: Stephen Kelly wrote: 7) I've added two commits to your branch. Please squash them into the appropriate commits in your topic. My bad, missed that one. Oh, forgot this one: 8) I get some unit test failures: The following tests FAILED: 85 - LinkFlags-dll_config (Failed) 87 - LinkFlags-exe_config (Failed) Working on this failure now. signature.asc Description: Message signed with OpenPGP using GPGMail -- 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
Re: [cmake-developers] push of LinkOptionsCommand topic branch
I have applied all the suggestions and re-pushed LinkOptionsCommand (after
rebasing) to stage.
Thanks,
SteveW
On Feb 2, 2014, at 2:44 AM, Stephen Kelly steve...@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
signature.asc
Description: Message signed with OpenPGP using GPGMail
--
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
Re: [cmake-developers] push of LinkOptionsCommand topic branch
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
[cmake-developers] push of LinkOptionsCommand topic branch
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, SteveW signature.asc Description: Message signed with OpenPGP using GPGMail -- 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
