Re: [cmake-developers] cmake -E capabilities [attempt 2]
Hi Brad, I will add the requested changes ASAP, but I have hardly any internet connectivity at this time, so it might take a while to get the necessary patches in shape and sent. Sorry for that! Best Regards, Tobias On Thu, Aug 4, 2016 at 4:25 PM, Brad King wrote: > On 08/03/2016 09:52 AM, Brad King wrote: >> I've revised Tobias's commits to rename NewFactory to GetFactory and >> explain some rationale in the commit message: >> >> Refactor extra generator registration to use factories >> https://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=a354f60c >> >> Report more information about extra generators in generator factories >> https://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=cd52a225 > [snip] > On 08/02/2016 02:30 PM, Brad King wrote: >> >> Once that tests cleanly I'll merge it to `master` and then we can >> rebase the remaining changes on it. > > The above commits are now in `master`. Please rebase the rest of > the topic. > > Please update Help/manual/cmake.1.rst to document the > `-E capabilities` option. Also please add a case to > Tests/RunCMake/CommandLine/RunCMakeTest.cmake to at least > run `cmake -E capabilities` as a smoke test. > > Is there a JSON convention for documenting schema? We will need > to be able to document the capabilities output format as well as > the protocol formats of cmake server mode. > > Thanks, > -Brad > -- Powered by www.kitware.com Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Kitware offers various services to support the CMake community. For more information on each offering, please visit: CMake Support: http://cmake.org/cmake/help/support.html CMake Consulting: http://cmake.org/cmake/help/consulting.html CMake Training Courses: http://cmake.org/cmake/help/training.html Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Follow this link to subscribe/unsubscribe: http://public.kitware.com/mailman/listinfo/cmake-developers
Re: [cmake-developers] cmake -E capabilities [attempt 2]
On 08/03/2016 09:52 AM, Brad King wrote: > I've revised Tobias's commits to rename NewFactory to GetFactory and > explain some rationale in the commit message: > > Refactor extra generator registration to use factories > https://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=a354f60c > > Report more information about extra generators in generator factories > https://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=cd52a225 [snip] On 08/02/2016 02:30 PM, Brad King wrote: > > Once that tests cleanly I'll merge it to `master` and then we can > rebase the remaining changes on it. The above commits are now in `master`. Please rebase the rest of the topic. Please update Help/manual/cmake.1.rst to document the `-E capabilities` option. Also please add a case to Tests/RunCMake/CommandLine/RunCMakeTest.cmake to at least run `cmake -E capabilities` as a smoke test. Is there a JSON convention for documenting schema? We will need to be able to document the capabilities output format as well as the protocol formats of cmake server mode. Thanks, -Brad -- Powered by www.kitware.com Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Kitware offers various services to support the CMake community. For more information on each offering, please visit: CMake Support: http://cmake.org/cmake/help/support.html CMake Consulting: http://cmake.org/cmake/help/consulting.html CMake Training Courses: http://cmake.org/cmake/help/training.html Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Follow this link to subscribe/unsubscribe: http://public.kitware.com/mailman/listinfo/cmake-developers
Re: [cmake-developers] cmake -E capabilities [attempt 2]
On 08/02/2016 04:48 PM, Stephen Kelly wrote: > The NewFactory methods in your patch don't return a new'd object, but > instead return static locals. The regular generators NewFactory methods > don't work that way, so you're introducing a pattern which is different to > what already exists and the commit message doesn't say why. I've revised Tobias's commits to rename NewFactory to GetFactory and explain some rationale in the commit message: Refactor extra generator registration to use factories https://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=a354f60c Report more information about extra generators in generator factories https://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=cd52a225 I don't think the granularity is too low for these particular changes. The commit only looks large because it essentially updates the syntax used to express a table of information. Perhaps this syntax update and the main logic updates could be separated but IMO it's good enough. -Brad -- Powered by www.kitware.com Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Kitware offers various services to support the CMake community. For more information on each offering, please visit: CMake Support: http://cmake.org/cmake/help/support.html CMake Consulting: http://cmake.org/cmake/help/consulting.html CMake Training Courses: http://cmake.org/cmake/help/training.html Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Follow this link to subscribe/unsubscribe: http://public.kitware.com/mailman/listinfo/cmake-developers
Re: [cmake-developers] cmake -E capabilities [attempt 2]
Tobias Hunger wrote:
> Hi Stephen,
>
> thanks for taking the time to do such a thorough review!
Actually my review wasn't thorough at all. I didn't try to review it
further.
The NewFactory methods in your patch don't return a new'd object, but
instead return static locals. The regular generators NewFactory methods
don't work that way, so you're introducing a pattern which is different to
what already exists and the commit message doesn't say why. Maybe there's a
reason, but I don't know what it is. In the future, no one else will know
either.
There are lots of things in there for which your intent is unclear. That's
why splitting and writing descriptive commit messages is valuable. You might
find
https://vimeo.com/172882423
to be a good introduction to this.
I have more to ask about your first commit (and why the second commit is
split out). I stopped my review at recommending it be split to see what it
is hiding.
> Added const to some of them:-) Hope I caught all.
cmake::ReportCapabilities() should be const, right?
>> * and a whitespace change that should be squashed into the commit that
>> introduces it
>
> I used Utilities/Scripts/clang-format.bash to do the formatting, so
> that should not be an issue. I just reran that on all commits. Maybe I
> forgot it in a commit or something before.
Maybe. Running the script on all commits would be the fix anyway.
>> * The CMAKE_BUILD_WITH_CMAKE macro should be in cmcmd.cxx wrapping the
>> capabilities handling:
>>
>> #if defined(CMAKE_BUILD_WITH_CMAKE)
>> else if (args[1] == "capabilities") {
>> cmake cm;
>> std::cout << cm.ReportCapabilities();
>> return 0;
>> }
>> #endif
>
> Why?
Because it's a bit odd to return 0 if capabilities can't be reported. I
missed that the contents of that method don't compile in bootstrap mode
though without the define.
>> As it is, it is doing many different things, none of which are mentioned
>> in the commit message, and some of which it probably shouldn't be doing.
>>
>> For example renaming GetExtraGeneratorName to
>> GetExternalMakefileProjectGeneratorName is probably not needed. If you
>> really want to do it, then it should be in its own commit with its own
>> commit message which justifies the change. As it is, it adds noise to the
>> big commit and makes it harder to review. Minimal is always better with
>> commits which do refactoring like that.
>
> I undid that change. That is one of the things that I originally
> removed and then realized last minute that it is needed somehow. So I
> added it, not realizing that I had removed similar functionality
> earlier.
This doesn't happen if you split commits. Splitting makes these things
visible to you and you can decide whether they were intentional or not at
that point.
>> A general good rule of thumb (which helps reviews, and makes things
>> bisectable in the future) is to do one thing per commit.
>
> I agree that this is the ideal we all should all strive for, but you
> are not going to get that from me anytime soon.
Calling it an 'ideal' isn't really the right mindset. It's easy, and it's
generally done by all other cmake contributors.
> At least not in the cmake codebase.
I don't know what this part means. It seems somehow disdainful, but I might
be missing something. Something to talk about in person perhaps.
Thanks,
Steve.
--
Powered by www.kitware.com
Please keep messages on-topic and check the CMake FAQ at:
http://www.cmake.org/Wiki/CMake_FAQ
Kitware offers various services to support the CMake community. For more
information on each offering, please visit:
CMake Support: http://cmake.org/cmake/help/support.html
CMake Consulting: http://cmake.org/cmake/help/consulting.html
CMake Training Courses: http://cmake.org/cmake/help/training.html
Visit other Kitware open-source projects at
http://www.kitware.com/opensource/opensource.html
Follow this link to subscribe/unsubscribe:
http://public.kitware.com/mailman/listinfo/cmake-developers
Re: [cmake-developers] cmake -E capabilities [attempt 2]
Tobias Hunger wrote:
> Hi Stephen,
>
> thanks for taking the time to do such a thorough review!
Actually my review wasn't thorough at all. I didn't try to review the branch
further.
The NewFactory methods in your patch don't return a new'd object, but
instead return static locals. The regular generators NewFactory methods
don't work that way, so you're introducing a pattern which is different to
what already exists and the commit message doesn't say why. Maybe there's a
reason, but I don't know what it is. In the future, no one else will know
either.
There are lots of things in there for which your intent is unclear. That's
why splitting and writing descriptive commit messages is valuable. You might
find
https://vimeo.com/172882423
to be a good introduction to this.
I have more to ask about your first commit (and why the second commit is
split out). I stopped my review at recommending it be split to see what it
is hiding.
> Added const to some of them:-) Hope I caught all.
cmake::ReportCapabilities() should be const, right?
>> * and a whitespace change that should be squashed into the commit that
>> introduces it
>
> I used Utilities/Scripts/clang-format.bash to do the formatting, so
> that should not be an issue. I just reran that on all commits. Maybe I
> forgot it in a commit or something before.
Maybe. Running the script on all commits would be the fix anyway.
>> * The CMAKE_BUILD_WITH_CMAKE macro should be in cmcmd.cxx wrapping the
>> capabilities handling:
>>
>> #if defined(CMAKE_BUILD_WITH_CMAKE)
>> else if (args[1] == "capabilities") {
>> cmake cm;
>> std::cout << cm.ReportCapabilities();
>> return 0;
>> }
>> #endif
>
> Why?
Because it's a bit odd to return 0 if capabilities can't be reported. I
missed that the contents of that method don't compile in bootstrap mode
though without the define.
>> As it is, it is doing many different things, none of which are mentioned
>> in the commit message, and some of which it probably shouldn't be doing.
>>
>> For example renaming GetExtraGeneratorName to
>> GetExternalMakefileProjectGeneratorName is probably not needed. If you
>> really want to do it, then it should be in its own commit with its own
>> commit message which justifies the change. As it is, it adds noise to the
>> big commit and makes it harder to review. Minimal is always better with
>> commits which do refactoring like that.
>
> I undid that change. That is one of the things that I originally
> removed and then realized last minute that it is needed somehow. So I
> added it, not realizing that I had removed similar functionality
> earlier.
This doesn't happen if you split commits. Splitting makes these things
visible to you and you can decide whether they were intentional or not at
that point.
>> A general good rule of thumb (which helps reviews, and makes things
>> bisectable in the future) is to do one thing per commit.
>
> I agree that this is the ideal we all should all strive for, but you
> are not going to get that from me anytime soon.
Calling it an 'ideal' isn't really the right mindset. It's easy, and it's
generally done by all other cmake contributors.
> At least not in the cmake codebase.
I don't know what this part means. It seems somehow disdainful, but I might
be missing something. Something to talk about in person perhaps.
Thanks,
Steve.
--
Powered by www.kitware.com
Please keep messages on-topic and check the CMake FAQ at:
http://www.cmake.org/Wiki/CMake_FAQ
Kitware offers various services to support the CMake community. For more
information on each offering, please visit:
CMake Support: http://cmake.org/cmake/help/support.html
CMake Consulting: http://cmake.org/cmake/help/consulting.html
CMake Training Courses: http://cmake.org/cmake/help/training.html
Visit other Kitware open-source projects at
http://www.kitware.com/opensource/opensource.html
Follow this link to subscribe/unsubscribe:
http://public.kitware.com/mailman/listinfo/cmake-developers
Re: [cmake-developers] cmake -E capabilities [attempt 2]
On 07/29/2016 06:01 AM, Tobias Hunger wrote: > I pushed an update that takes most of the feedback into account. Thanks. I've applied the extra generator refactoring parts first: Refactor extra generator registration to use factories https://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=bc44627b Report more information about extra generators in generator factories https://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=034caa27 Once that tests cleanly I'll merge it to `master` and then we can rebase the remaining changes on it. Thanks, -Brad -- Powered by www.kitware.com Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Kitware offers various services to support the CMake community. For more information on each offering, please visit: CMake Support: http://cmake.org/cmake/help/support.html CMake Consulting: http://cmake.org/cmake/help/consulting.html CMake Training Courses: http://cmake.org/cmake/help/training.html Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Follow this link to subscribe/unsubscribe: http://public.kitware.com/mailman/listinfo/cmake-developers
Re: [cmake-developers] cmake -E capabilities [attempt 2]
Hi Stephen,
thanks for taking the time to do such a thorough review!
I pushed an update that takes most of the feedback into account. Still at:
https://github.com/hunger/CMake/commits/cmake-capabilities
On Wed, Jul 27, 2016 at 1:11 AM, Stephen Kelly wrote:
> Tobias Hunger wrote:
>
>> Did anyone find some time for a review yet?
>
> Hi Tobias,
>
> I had a look through this this evening. Thanks for working on this. The
> commit adding the functionality at the end looks much better after the extra
> generator refactoring.
>
> Here are some review notes:
>
> * That commit has a cmDefinitions include though that should be removed.
Gone.
> * There are also some methods that should be const
Added const to some of them:-) Hope I caught all.
> * and a whitespace change that should be squashed into the commit that
> introduces it
I used Utilities/Scripts/clang-format.bash to do the formatting, so
that should not be an issue. I just reran that on all commits. Maybe I
forgot it in a commit or something before.
> * The pretty-print flag should be removed. Aside from being a boolean trap,
> it creates a interface segregation principle violation. See eg
Removed, even though I find it really useful when testing cmake on
mac/windows. I do have all the small helper tools on Linux, but
usually not on windows/Mac.
> * The CMAKE_BUILD_WITH_CMAKE macro should be in cmcmd.cxx wrapping the
> capabilities handling:
>
> #if defined(CMAKE_BUILD_WITH_CMAKE)
> else if (args[1] == "capabilities") {
> cmake cm;
> std::cout << cm.ReportCapabilities();
> return 0;
> }
> #endif
Why?
> That define should not be used in ReportCapabilities.
That define seems necessary to keep the bootstrap test running.
> * Splitting the 'CMake: Refactor ExtraGenerator registration' commit into
> multiple commits would make it more reviewable, and more bisectable in the
> future.
Sorry, this is what you will get from me: I have to start at some
place and meddle through till things work again. I have to back-track
often.
Basically I need to get some functionality implemented completely and
then test that. Only afterwards can I spend time on making the patches
pretty.
If that is not acceptable, then please feel free to do necessary
changes yourself.
> As it is, it is doing many different things, none of which are mentioned in
> the commit message, and some of which it probably shouldn't be doing.
>
> For example renaming GetExtraGeneratorName to
> GetExternalMakefileProjectGeneratorName is probably not needed. If you
> really want to do it, then it should be in its own commit with its own
> commit message which justifies the change. As it is, it adds noise to the
> big commit and makes it harder to review. Minimal is always better with
> commits which do refactoring like that.
I undid that change. That is one of the things that I originally
removed and then realized last minute that it is needed somehow. So I
added it, not realizing that I had removed similar functionality
earlier.
> A general good rule of thumb (which helps reviews, and makes things
> bisectable in the future) is to do one thing per commit.
I agree that this is the ideal we all should all strive for, but you
are not going to get that from me anytime soon. At least not in the
cmake codebase.
Best Regards,
Tobias
--
Powered by www.kitware.com
Please keep messages on-topic and check the CMake FAQ at:
http://www.cmake.org/Wiki/CMake_FAQ
Kitware offers various services to support the CMake community. For more
information on each offering, please visit:
CMake Support: http://cmake.org/cmake/help/support.html
CMake Consulting: http://cmake.org/cmake/help/consulting.html
CMake Training Courses: http://cmake.org/cmake/help/training.html
Visit other Kitware open-source projects at
http://www.kitware.com/opensource/opensource.html
Follow this link to subscribe/unsubscribe:
http://public.kitware.com/mailman/listinfo/cmake-developers
Re: [cmake-developers] cmake -E capabilities [attempt 2]
Tobias Hunger wrote:
> Did anyone find some time for a review yet?
Hi Tobias,
I had a look through this this evening. Thanks for working on this. The
commit adding the functionality at the end looks much better after the extra
generator refactoring.
Here are some review notes:
* That commit has a cmDefinitions include though that should be removed.
* There are also some methods that should be const
* and a whitespace change that should be squashed into the commit that
introduces it
* The pretty-print flag should be removed. Aside from being a boolean trap,
it creates a interface segregation principle violation. See eg
https://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=23f87e81
for more. If you want to pretty print things on the command line I suggest
cmake -E capabilities | jq
which will give you colorized output, or
cmake -E capabilities | python -mjson.tool
for something that you already have installed. See
http://stackoverflow.com/questions/352098/how-can-i-pretty-print-json
for more.
* The CMAKE_BUILD_WITH_CMAKE macro should be in cmcmd.cxx wrapping the
capabilities handling:
#if defined(CMAKE_BUILD_WITH_CMAKE)
else if (args[1] == "capabilities") {
cmake cm;
std::cout << cm.ReportCapabilities();
return 0;
}
#endif
That define should not be used in ReportCapabilities.
* Splitting the 'CMake: Refactor ExtraGenerator registration' commit into
multiple commits would make it more reviewable, and more bisectable in the
future.
As it is, it is doing many different things, none of which are mentioned in
the commit message, and some of which it probably shouldn't be doing.
For example renaming GetExtraGeneratorName to
GetExternalMakefileProjectGeneratorName is probably not needed. If you
really want to do it, then it should be in its own commit with its own
commit message which justifies the change. As it is, it adds noise to the
big commit and makes it harder to review. Minimal is always better with
commits which do refactoring like that.
A general good rule of thumb (which helps reviews, and makes things
bisectable in the future) is to do one thing per commit.
Thanks,
Steve.
--
Powered by www.kitware.com
Please keep messages on-topic and check the CMake FAQ at:
http://www.cmake.org/Wiki/CMake_FAQ
Kitware offers various services to support the CMake community. For more
information on each offering, please visit:
CMake Support: http://cmake.org/cmake/help/support.html
CMake Consulting: http://cmake.org/cmake/help/consulting.html
CMake Training Courses: http://cmake.org/cmake/help/training.html
Visit other Kitware open-source projects at
http://www.kitware.com/opensource/opensource.html
Follow this link to subscribe/unsubscribe:
http://public.kitware.com/mailman/listinfo/cmake-developers
Re: [cmake-developers] cmake -E capabilities [attempt 2]
Did anyone find some time for a review yet? Best regards, Tobias Am 22.07.2016 01:37 schrieb "Tobias Hunger" : > Hello fellow developers, > > https://github.com/hunger/CMake/commits/cmake-capabilities > > has my attempt to get "cmake -E capabilities" merged -- in preparation of > the > server mode. > > Brad already merged a big chunk of the changes, but he wanted to see the > extra > generator registration cleaned up before continuing. That has happened in > the > new push: The first patch adds a cmExternalMakefileProjectGeneratorFactory, > which has all the necessary information so that cmake no longer needs to > create > instances of all extra generators during startup. > > Extra generators can now alias others -- which is used for the KDevelop3 > generator. Maybe we can just remove that? The comments claim the generator > is > for compatibility with cmake 2.2. That would shave a couple of lines of > that > patch... > > The capabilities work on top of that branch is mostly unchanged, but > exposes an > "isAlias" flag. I now suppress all aliases in the output of -E > capabilities as > I think it makes no sense to have new code announce compatibility bridges > (and > the last version already hardcoded KDevelop3 to be suppressed). > > Best Regards, > Tobias > > -- > Tobias Hunger, Senior Software Engineer | The Qt Company > The Qt Company GmbH, Rudower Chaussee 13, D-12489 Berlin > Geschäftsführer: Mika Pälsi, Juha Varelius, Mika Harjuaho. Sitz der > Gesellschaft: Berlin, Registergericht: Amtsgericht Charlottenburg, HRB > 144331 B > -- > > Powered by www.kitware.com > > Please keep messages on-topic and check the CMake FAQ at: > http://www.cmake.org/Wiki/CMake_FAQ > > Kitware offers various services to support the CMake community. For more > information on each offering, please visit: > > CMake Support: http://cmake.org/cmake/help/support.html > CMake Consulting: http://cmake.org/cmake/help/consulting.html > CMake Training Courses: http://cmake.org/cmake/help/training.html > > Visit other Kitware open-source projects at > http://www.kitware.com/opensource/opensource.html > > Follow this link to subscribe/unsubscribe: > http://public.kitware.com/mailman/listinfo/cmake-developers -- Powered by www.kitware.com Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Kitware offers various services to support the CMake community. For more information on each offering, please visit: CMake Support: http://cmake.org/cmake/help/support.html CMake Consulting: http://cmake.org/cmake/help/consulting.html CMake Training Courses: http://cmake.org/cmake/help/training.html Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Follow this link to subscribe/unsubscribe: http://public.kitware.com/mailman/listinfo/cmake-developers
