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] Autogen subdirectories patches
Sebastian Holtermann wrote: >> It might be better to just use a NULL sentinel at the end of the array. >> If not, the extra spaces here should go away. > > I have changed that to use a NULL sentinel. > std::array would be the best solution IMO but it is not allowed, is it? You could use cmArrayBegin and cmArrayEnd without the sentinal. They are designed for this kind of thing. 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] Autogen subdirectories patches
Thanks for the feedback.
I have prepared an other approach which uses checksum suffixes
instead of nested directory generation.
This was tested on my Debian/amd64/Makefile setup.
Interesting; I like it. Might be worthwhile to reuse it for the .o files
in the future as well (since these can conflict, but are much less
likely to do so).
Please review.
Could you run clang-format over the code? There are a number of style
issues that it will fix (IIRC, it needs at least Clang 3.8; I'm unsure
what version of the clang that comes with Xcode is sufficient).
I didn't knew clang-format. That's a powerful tool!
I have applied clang-format-3.8 which is available in Debian/Stretch.
+rccOutputPath += cmQtAutoGeneratorUtil::BuildFileBase( qrcSourceName,
+ makefile->GetCurrentSourceDirectory(),
+ makefile->GetCurrentBinaryDirectory(),
+ makefile->GetHomeDirectory(),
+ makefile->GetHomeOutputDirectory() );
Is there a reason we can't pass just the `makefile` down and have it
query all the parameters rather than having 5 arguments?
In fact there is.
cmQtAutoGeneratorUtil::BuildFileBase is shared between
cmQtAutoGeneratorInitializer and cmQtAutoGenerators.
Both hold the directory names in different variables
which is why they must be passed manually.
+ struct TPair {
+const std::string * dir;
+const char * seed;
+ };
+ TPair pDirs[4] = {
+{ &parenDirCSource, "CurrentSource" },
+{ &parenDirCBinary, "CurrentBinary" },
+{ &parenDirPSource, "ProjectSource" },
+{ &parenDirPBinary, "ProjectBinary" }
+ };
+ TPair * itc = &pDirs[0];
+ TPair * ite = &pDirs[0] + ( sizeof ( pDirs ) / sizeof ( TPair ) );
It might be better to just use a NULL sentinel at the end of the array.
If not, the extra spaces here should go away.
I have changed that to use a NULL sentinel.
std::array would be the best solution IMO but it is not allowed, is it?
+ // Calculate the file ( seed + relative path + name ) checksum
+ std::string checksumBase64;
+ {
+::std::vectorhashBytes;
CMake just uses std::, not ::std::.
Changed.
+{
+ // Acquire hex value hash string
+ std::string hexHash = cmCryptoHash::New("SHA256")->HashString(
+(sourceRelSeed+sourceRelPath+sourceFileName).c_str());
+ // Convert hex value string to bytes
+ hashBytes.resize ( hexHash.size()/2 );
+ for ( unsigned int ii=0; ii != hashBytes.size(); ++ii ) {
+unsigned char byte ( 0 );
+for ( unsigned int jj=0; jj != 2; ++jj ) {
+ unsigned char cval ( 0 );
+ switch ( hexHash[ii*2+jj] ) {
+ case '0': cval=0; break; case '1': cval=1; break;
+ case '2': cval=2; break; case '3': cval=3; break;
+ case '4': cval=4; break; case '5': cval=5; break;
+ case '6': cval=6; break; case '7': cval=7; break;
+ case '8': cval=8; break; case '9': cval=9; break;
+ case 'a': cval=10; break; case 'b': cval=11; break;
+ case 'c': cval=12; break; case 'd': cval=13; break;
+ case 'e': cval=14; break; case 'f': cval=15; break;
+ default: break;
I feel like this is better as:
int nibble = hexHash[ii * 2 + jj];
if ('0' <= nibble && nibble <= '9') {
cval = nibble - '0';
} else if ('a' <= nibble && nibble <= 'f') {
cval = nibble - 'a' + 10;
} else {
// internal error about an unexpected hexchar
}
Alternatively, cmUuid::IntFromHexDigit() (and possibly other methods)
could be refactored to allow this to share an implementation.
I've changed it to the nibble version.
There also was a bit shift error (1 vs 4).
Doing so I found that Base64 allows '+' and '/' as characters which is
bad for directory names obviously.
For now these characters get replaced with 'A' and 'B'.
If this technique gets wider use using Base58 might be a better choice.
But it is not available in CMake, yet.
-Sebastian
>From 21c8b1f478c598c244cebab8f6f60956ecc51de0 Mon Sep 17 00:00:00 2001
From: Sebastian Holtermann
Date: Tue, 26 Jul 2016 16:39:12 +0200
Subject: [PATCH] QtAutogen fix for too deep nested directory generation.
Instead of generating moc_* and qrc_* files in subdirectories
that reflect their source's location in the source tree
the files get generated solely in the TARGET_NAME_automoc.dir/
but get a Base64 encoded checksum suffix that was generated
from their source path and a few more seed strings.
---
Source/cmQtAutoGeneratorInitializer.cxx | 79 +++-
Source/cmQtAutoGenerators.cxx | 211 +++-
Source/cmQtAutoGenerators.h | 21 +++-
3 files changed, 188 insertions(+), 123 deletions(-)
diff --git a/Source/cmQtAutoGeneratorInitializer.cxx b/Source/cmQtAutoGeneratorInitializer.cxx
index dd19760..6bbe29c 100644
--- a/Source/cmQtAutoGeneratorInitializer.cxx
+++ b/Source/cmQtAutoGeneratorInitializer.cxx
@@ -12,6 +12,7 @@
=
Re: [cmake-developers] Autogen subdirectories patches
On Tue, Jul 26, 2016 at 17:31:00 +0200, Sebastian Holtermann wrote:
> I have prepared an other approach which uses checksum suffixes
> instead of nested directory generation.
> This was tested on my Debian/amd64/Makefile setup.
Interesting; I like it. Might be worthwhile to reuse it for the .o files
in the future as well (since these can conflict, but are much less
likely to do so).
> Please review.
Could you run clang-format over the code? There are a number of style
issues that it will fix (IIRC, it needs at least Clang 3.8; I'm unsure
what version of the clang that comes with Xcode is sufficient).
> +rccOutputPath += cmQtAutoGeneratorUtil::BuildFileBase( qrcSourceName,
> + makefile->GetCurrentSourceDirectory(),
> + makefile->GetCurrentBinaryDirectory(),
> + makefile->GetHomeDirectory(),
> + makefile->GetHomeOutputDirectory() );
Is there a reason we can't pass just the `makefile` down and have it
query all the parameters rather than having 5 arguments?
> + struct TPair {
> +const std::string * dir;
> +const char * seed;
> + };
> + TPair pDirs[4] = {
> +{ &parenDirCSource, "CurrentSource" },
> +{ &parenDirCBinary, "CurrentBinary" },
> +{ &parenDirPSource, "ProjectSource" },
> +{ &parenDirPBinary, "ProjectBinary" }
> + };
> + TPair * itc = &pDirs[0];
> + TPair * ite = &pDirs[0] + ( sizeof ( pDirs ) / sizeof ( TPair ) );
It might be better to just use a NULL sentinel at the end of the array.
If not, the extra spaces here should go away.
> + // Calculate the file ( seed + relative path + name ) checksum
> + std::string checksumBase64;
> + {
> +::std::vectorhashBytes;
CMake just uses std::, not ::std::.
> +{
> + // Acquire hex value hash string
> + std::string hexHash = cmCryptoHash::New("SHA256")->HashString(
> +(sourceRelSeed+sourceRelPath+sourceFileName).c_str());
> + // Convert hex value string to bytes
> + hashBytes.resize ( hexHash.size()/2 );
> + for ( unsigned int ii=0; ii != hashBytes.size(); ++ii ) {
> +unsigned char byte ( 0 );
> +for ( unsigned int jj=0; jj != 2; ++jj ) {
> + unsigned char cval ( 0 );
> + switch ( hexHash[ii*2+jj] ) {
> + case '0': cval=0; break; case '1': cval=1; break;
> + case '2': cval=2; break; case '3': cval=3; break;
> + case '4': cval=4; break; case '5': cval=5; break;
> + case '6': cval=6; break; case '7': cval=7; break;
> + case '8': cval=8; break; case '9': cval=9; break;
> + case 'a': cval=10; break; case 'b': cval=11; break;
> + case 'c': cval=12; break; case 'd': cval=13; break;
> + case 'e': cval=14; break; case 'f': cval=15; break;
> + default: break;
I feel like this is better as:
int nibble = hexHash[ii * 2 + jj];
if ('0' <= nibble && nibble <= '9') {
cval = nibble - '0';
} else if ('a' <= nibble && nibble <= 'f') {
cval = nibble - 'a' + 10;
} else {
// internal error about an unexpected hexchar
}
Alternatively, cmUuid::IntFromHexDigit() (and possibly other methods)
could be refactored to allow this to share an implementation.
--Ben
--
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] Autogen subdirectories patches
Am 21.07.2016 um 17:40 schrieb Brad King:
On 04/22/2016 09:37 AM, Brad King wrote:
The rest of the changes are now in 'master'. Thanks for working on this!
Unfortunately I've had to revert some of these changes due to
regressions they cause. See issue here:
https://gitlab.kitware.com/cmake/cmake/issues/16209
Hi,
I have prepared an other approach which uses checksum suffixes
instead of nested directory generation.
This was tested on my Debian/amd64/Makefile setup.
-- From the commit:
Subject: [PATCH] QtAutogen fix for too deep nested directory generation.
Instead of generating moc_* and qrc_* files in subdirectories
that reflect their source's location in the source tree
the files get generated solely in the TARGET_NAME_automoc.dir/
but get a Base64 encoded checksum suffix that was generated
from their source path and a few more seed strings.
--
Please review.
-Sebastian
>From 3cdb41e57e45ae145066b18a00aac53c9801b332 Mon Sep 17 00:00:00 2001
From: Sebastian Holtermann
Date: Tue, 26 Jul 2016 16:39:12 +0200
Subject: [PATCH] QtAutogen fix for too deep nested directory generation.
Instead of generating moc_* and qrc_* files in subdirectories
that reflect their source's location in the source tree
the files get generated solely in the TARGET_NAME_automoc.dir/
but get a Base64 encoded checksum suffix that was generated
from their source path and a few more seed strings.
---
Source/cmQtAutoGeneratorInitializer.cxx | 80 +++-
Source/cmQtAutoGenerators.cxx | 217 +++-
Source/cmQtAutoGenerators.h | 26 +++-
3 files changed, 202 insertions(+), 121 deletions(-)
diff --git a/Source/cmQtAutoGeneratorInitializer.cxx b/Source/cmQtAutoGeneratorInitializer.cxx
index dd19760..fcc9367 100644
--- a/Source/cmQtAutoGeneratorInitializer.cxx
+++ b/Source/cmQtAutoGeneratorInitializer.cxx
@@ -12,6 +12,7 @@
*/
#include "cmQtAutoGeneratorInitializer.h"
+#include "cmQtAutoGenerators.h"
#include "cmLocalGenerator.h"
#include "cmMakefile.h"
@@ -53,49 +54,25 @@ static std::string GetAutogenTargetBuildDir(cmGeneratorTarget const* target)
return targetDir;
}
-static std::string GetSourceRelativePath(cmGeneratorTarget const* target,
- const std::string& fileName)
+static std::string GetQrcBuildPath(cmGeneratorTarget const* target,
+ const std::string& qrcSourceName)
{
- std::string pathRel;
- // Test if the file is child to any of the known directories
+ std::string rccOutputPath = GetAutogenTargetBuildDir(target);
+ // Create output directory
+ cmSystemTools::MakeDirectory(rccOutputPath.c_str());
+
+ rccOutputPath += "qrc_";
{
-const std::string fileNameReal = cmsys::SystemTools::GetRealPath(fileName);
-std::string parentDirectory;
-bool match(false);
-{
- std::string testDirs[4];
- {
-cmMakefile* makefile = target->Target->GetMakefile();
-testDirs[0] = makefile->GetCurrentSourceDirectory();
-testDirs[1] = makefile->GetCurrentBinaryDirectory();
-testDirs[2] = makefile->GetHomeDirectory();
-testDirs[3] = makefile->GetHomeOutputDirectory();
- }
- for (int ii = 0; ii != sizeof(testDirs) / sizeof(std::string); ++ii) {
-const ::std::string testDir =
- cmsys::SystemTools::GetRealPath(testDirs[ii]);
-if (!testDir.empty() &&
-cmsys::SystemTools::IsSubDirectory(fileNameReal, testDir)) {
- parentDirectory = testDir;
- match = true;
- break;
-}
- }
-}
-// Use root as fallback parent directory
-if (!match) {
- cmsys::SystemTools::SplitPathRootComponent(fileNameReal,
- &parentDirectory);
-}
-pathRel = cmsys::SystemTools::RelativePath(
- parentDirectory, cmsys::SystemTools::GetParentDirectory(fileNameReal));
+cmMakefile* makefile = target->Target->GetMakefile();
+rccOutputPath += cmQtAutoGeneratorUtil::BuildFileBase( qrcSourceName,
+ makefile->GetCurrentSourceDirectory(),
+ makefile->GetCurrentBinaryDirectory(),
+ makefile->GetHomeDirectory(),
+ makefile->GetHomeOutputDirectory() );
}
- // Sanitize relative path
- if (!pathRel.empty()) {
-pathRel += '/';
-cmSystemTools::ReplaceString(pathRel, "..", "__");
- }
- return pathRel;
+ rccOutputPath += ".cpp";
+
+ return rccOutputPath;
}
static void SetupSourceFiles(cmGeneratorTarget const* target,
@@ -129,15 +106,7 @@ static void SetupSourceFiles(cmGeneratorTarget const* target,
if (ext == "qrc" &&
!cmSystemTools::IsOn(sf->GetPropertyForUser("SKIP_AUTORCC"))) {
-std::string rcc_output_dir = GetAutogenTargetBuildDir(target);
-rcc_output_dir += GetSourceRelativePath(target, absFile);
-cmSystemTools::MakeDirectory(rcc_output_dir.c_str());
-
-
Re: [cmake-developers] Question over language bindings / daemon
Hi Rick, this is something I played with a bit, and while it is completely possible to make your own little python binding, it will not be merged into CMake, which means you will have to maintain it on your side. Also, you will need to write the whole interface yourself, as the current CMake lib API is not meant to be used in such a way. While I too am more familiar and at ease with python and its ecosystem, lua is a better fit for CMake as it can be redistribued easily (just one C file to add to the build). This would allow for better control of the updates, avoiding potential conflicts with the installed version on the system and so on. My Proof of concept can be found easily, but it will remain in this unfinished state for ever, since when I'll get time to work on this I will pursue the Lua angle. Best, Charles Le lun. 18 juil. 2016 à 14:47, Tobias Hunger a écrit : > Hi Rick, > > On Mo, 2016-07-18 at 00:59 +0100, RW via cmake-developers wrote: > > Question 1 > > Will the new daemon mode if it's included have all the commands > associated > > with a CMakeFile? > > (In other-words could I write a python library that could just pipe > > commands into it, instead of generating a separate CMakeFile) > > It's server mode now:-) > > No, daemon mode will provide commands to query the build graph etc., but > it will > not be usable to feed CMakeLists.txt-commands to cmake one by one. > > > Question 2 > > I had a look at the way the functions are written within the code > > while I could extract a list of them using something like CppSharp (for > > example to extract a list of everything that inherits from cmCommand) > > There doesn't seem to be any metadata that describes arguments that are > > passed into a given function. (only that each function takes a vector of > > strings). > > Do you know if there's any xml or associated metadata (perhaps used to > > generate the docs) for which arguments are associated with a given > function? > > This in turn is in scope for server mode (not at the start, but later): > That > information is a by-product of the idea of server mode helping IDEs with > code- > completion of CMakeLists.txt files. > > 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
