Re: [cmake-developers] Autogen subdirectories patches

[Sebastian Holtermann]

Am 27.07.2016 um 19:31 schrieb Ben Boeckel:

On Wed, Jul 27, 2016 at 17:49:40 +0200, Sebastian Holtermann wrote:

Is there an error exit function in CMake?
Or just exit(-1);


Here's an example I found:

this->Makefile->IssueMessage(
  cmake::INTERNAL_ERROR, "fileFound is true but FileFound is empty!");

after that, it is treated as if an error occurred; this loop should
probably be bailed from.


Unfortunately the function is called in two places of which one
does not provide a Makefile (the cmake -E call).

It is very unlikely that an invalid character appears ever
and if it does it is likely that something else is very wrong.
In the patch I sent earlier in this case cmake just drops a note
via std::cerr and calls exit(-1). IMO this is sufficient.

-Sebastian


--

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]

Am 27.07.2016 um 17:49 schrieb Sebastian Holtermann:



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


'_' and '@' would be better replacements (with comments why they are
used) since 'A' and 'B' are already characters in Base64.


Some quick research (aka googling) revealed
http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_278

It seems '_' and '-' are safer choices.


+// unexpected hexchar


This raise an internal error, not just be a comment.


Is there an error exit function in CMake?
Or just exit(-1);


+  }
+}
+hashBytes[ii] = hbyte[0] | (hbyte[1] << 4);
+  }
+}
+// Convert hash bytes to Base64 text string
+{
+  std::vector base64Bytes(hashBytes.size() * 2, 0);
+  cmsysBase64_Encode([0], hashBytes.size(),
[0], 0);
+  checksumBase64 = reinterpret_cast([0]);
+  // Base64 allows '+' and '/' characters. Replace these.


This comment should indicate it is because the string is used as part of
a path and that these characters tend to cause problems in paths.


Ok.



The patch.

-Sebastian


>From 30d0c7b61a2ffbc9004cd32a88330078ec8d75a4 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   | 215 +++-
 Source/cmQtAutoGenerators.h |  21 +++-
 3 files changed, 192 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 @@
 */
 
 #include "cmQtAutoGeneratorInitializer.h"
+#include "cmQtAutoGenerators.h"
 
 #include "cmLocalGenerator.h"
 #include "cmMakefile.h"
@@ -53,49 +54,24 @@ 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,
- );
-}
-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 +105,7 @@ static void SetupSourceFiles(cmGeneratorTarget const* 

Re: [cmake-developers] Autogen subdirectories patches

[Sebastian Holtermann]

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


'_' and '@' would be better replacements (with comments why they are
used) since 'A' and 'B' are already characters in Base64.


Some quick research (aka googling) revealed
http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_278
It seems '_' and '-' are safer choices.


+// unexpected hexchar


This raise an internal error, not just be a comment.


Is there an error exit function in CMake?
Or just exit(-1);


+  }
+}
+hashBytes[ii] = hbyte[0] | (hbyte[1] << 4);
+  }
+}
+// Convert hash bytes to Base64 text string
+{
+  std::vector base64Bytes(hashBytes.size() * 2, 0);
+  cmsysBase64_Encode([0], hashBytes.size(), [0], 0);
+  checksumBase64 = reinterpret_cast([0]);
+  // Base64 allows '+' and '/' characters. Replace these.


This comment should indicate it is because the string is used as part of
a path and that these characters tend to cause problems in paths.


Ok.

-Sebastian

--

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

[Ben Boeckel]

On Tue, Jul 26, 2016 at 23:38:34 +0200, Sebastian Holtermann wrote:
> 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'.

'_' and '@' would be better replacements (with comments why they are
used) since 'A' and 'B' are already characters in Base64.

> +// unexpected hexchar

This raise an internal error, not just be a comment.

> +  }
> +}
> +hashBytes[ii] = hbyte[0] | (hbyte[1] << 4);
> +  }
> +}
> +// Convert hash bytes to Base64 text string
> +{
> +  std::vector base64Bytes(hashBytes.size() * 2, 0);
> +  cmsysBase64_Encode([0], hashBytes.size(), [0], 
> 0);
> +  checksumBase64 = reinterpret_cast([0]);
> +  // Base64 allows '+' and '/' characters. Replace these.

This comment should indicate it is because the string is used as part of
a path and that these characters tend to cause problems in paths.

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

[Stephen Kelly]

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

[Sebastian Holtermann]

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] = {
+{ , "CurrentSource" },
+{ , "CurrentBinary" },
+{ , "ProjectSource" },
+{ , "ProjectBinary" }
+  };
+  TPair * itc = [0];
+  TPair * ite = [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

[Ben Boeckel]

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] = {
> +{ , "CurrentSource" },
> +{ , "CurrentBinary" },
> +{ , "ProjectSource" },
> +{ , "ProjectBinary" }
> +  };
> +  TPair * itc = [0];
> +  TPair * ite = [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

[Sebastian Holtermann]

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,
- );
-}
-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] Autogen subdirectories patches

[Sebastian Holtermann]

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



I've seen it.
At the moment I'm busy but I'll try to look at it next week.

-Sebastian


--

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

[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

-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] Autogen subdirectories patches

[Sebastian Holtermann]

Am 22.04.2016 um 15:37 schrieb Brad King:

On 04/21/2016 03:14 AM, Sebastian Holtermann wrote:

Thanks!  I've applied them locally and merged the cleanup/refactoring
commits to 'next' for testing first.  Once those test cleanly I'll move
on to the rest.


It is good too see the patches made it into the next branch.


The rest of the changes are now in 'master'.  Thanks for working on this!


Now there is another issue I have a partially fix for.

As of now all included mocs and uis get generated in
CMAKE_CURRENT_BINARY_DIR/
because they must be within INCLUDE_DIRECTORIES.
I think a more robust approach would be to generate them in
CMAKE_CURRENT_BINARY_DIR/TARGET_automoc.dir/
and to add the _automoc.dir to INCLUDE_DIRECTORIES.

I've attached a patch that does does so
- it is relative to current "next" branch.


You should be able to rebase on 'master' now.


It is incomplete though because I didn't manage to get
CMAKE_CURRENT_BINARY_DIR/TARGET_automoc.dir/
into INCLUDE_DIRECTORIES in Source/cmQtAutoGeneratorInitializer.cxx.
(GetAutogenTargetBuildDir() in Source/cmQtAutoGeneratorInitializer.cxx)

If someone could do that or give me pointers on how to do it
then this could be another improvement.


I'm afraid I'm not familiar enough with this infrastructure to know
for sure.  The entire Qt autogen infrastructure was built by others.
Take a look at cmGlobalGenerator::Compute to see a carefully
ordered set of operations.  Among them is the QtAutoGenerator step.
I'm not sure of its order relative to INCLUDE_DIRECTORIES evaluation.


I see. Thanks for the pointer.
Unfortunately I can't spend much more time on this
right now but maybe later.
Thanks again for the kind support.

-Sebastian

--

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

[Brad King]

On 04/21/2016 03:14 AM, Sebastian Holtermann wrote:
>> Thanks!  I've applied them locally and merged the cleanup/refactoring
>> commits to 'next' for testing first.  Once those test cleanly I'll move
>> on to the rest.
> 
> It is good too see the patches made it into the next branch.

The rest of the changes are now in 'master'.  Thanks for working on this!

> Now there is another issue I have a partially fix for.
> 
> As of now all included mocs and uis get generated in
> CMAKE_CURRENT_BINARY_DIR/
> because they must be within INCLUDE_DIRECTORIES.
> I think a more robust approach would be to generate them in
> CMAKE_CURRENT_BINARY_DIR/TARGET_automoc.dir/
> and to add the _automoc.dir to INCLUDE_DIRECTORIES.
> 
> I've attached a patch that does does so
> - it is relative to current "next" branch.

You should be able to rebase on 'master' now.

> It is incomplete though because I didn't manage to get
> CMAKE_CURRENT_BINARY_DIR/TARGET_automoc.dir/
> into INCLUDE_DIRECTORIES in Source/cmQtAutoGeneratorInitializer.cxx.
> (GetAutogenTargetBuildDir() in Source/cmQtAutoGeneratorInitializer.cxx)
> 
> If someone could do that or give me pointers on how to do it
> then this could be another improvement.

I'm afraid I'm not familiar enough with this infrastructure to know
for sure.  The entire Qt autogen infrastructure was built by others.
Take a look at cmGlobalGenerator::Compute to see a carefully
ordered set of operations.  Among them is the QtAutoGenerator step.
I'm not sure of its order relative to INCLUDE_DIRECTORIES evaluation.

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] Autogen subdirectories patches

[Sebastian Holtermann]

Am 19.04.2016 um 19:28 schrieb Brad King:

On 04/19/2016 11:09 AM, Sebastian Holtermann wrote:

https://cmake.org/Bug/view.php?id=12873
https://cmake.org/Bug/view.php?id=16068

They introduce
- same name collision checks during moc/qrc/ui generation
- moc/qrc generation in subdirectories to support
sources with the name in different subdirectories
- A test for equally named sources in different subdirectories

Please review.


Thanks!  I've applied them locally and merged the cleanup/refactoring
commits to 'next' for testing first.  Once those test cleanly I'll move
on to the rest.


It is good too see the patches made it into the next branch.
Thanks and sorry for the wrong indentation btw..


Now there is another issue I have a partially fix for.

As of now all included mocs and uis get generated in
CMAKE_CURRENT_BINARY_DIR/
because they must be within INCLUDE_DIRECTORIES.
I think a more robust approach would be to generate them in
CMAKE_CURRENT_BINARY_DIR/TARGET_automoc.dir/
and to add the _automoc.dir to INCLUDE_DIRECTORIES.

I've attached a patch that does does so
- it is relative to current "next" branch.

It is incomplete though because I didn't manage to get
CMAKE_CURRENT_BINARY_DIR/TARGET_automoc.dir/
into INCLUDE_DIRECTORIES in Source/cmQtAutoGeneratorInitializer.cxx.
(GetAutogenTargetBuildDir() in Source/cmQtAutoGeneratorInitializer.cxx)

If someone could do that or give me pointers on how to do it
then this could be another improvement.

-Sebastian

>From aa246261ba1f98ca245f8c5a93b6c93c7fbe8ec5 Mon Sep 17 00:00:00 2001
From: Sebastian Holtermann 
Date: Thu, 21 Apr 2016 08:44:11 +0200
Subject: [PATCH] Autogen: Generate included moc and ui files in
 TARGET_automoc.dir/

---
 Source/cmQtAutoGenerators.cxx | 17 -
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/Source/cmQtAutoGenerators.cxx b/Source/cmQtAutoGenerators.cxx
index c6ee751..29e2e47 100644
--- a/Source/cmQtAutoGenerators.cxx
+++ b/Source/cmQtAutoGenerators.cxx
@@ -564,6 +564,13 @@ bool cmQtAutoGenerators::RunAutogen(cmMakefile* makefile)
 this->SearchHeadersForCppFile(absFilename, headerExtensions, headerFiles);
 }
 
+  // Prepend included mocs with build subdirectory
+  for(std::map::iterator
+  it = includedMocs.begin(); it != includedMocs.end(); ++it)
+{
+  it->second = this->TargetBuildSubDir + it->second;
+}
+
   {
   std::vector mocSkipped;
   cmSystemTools::ExpandListArgument(this->SkipMoc, mocSkipped);
@@ -1261,7 +1268,8 @@ bool cmQtAutoGenerators::GenerateUiFiles(
   {
   const std::string & uiFileName = *sit;
   const std::string uiInputFile = sourcePath + uiFileName + ".ui";
-  const std::string uiOutputFile = "ui_" + uiFileName + ".h";
+  const std::string uiOutputFile = this->TargetBuildSubDir
+   + "ui_" + uiFileName + ".h";
   sourceMap[uiInputFile] = uiOutputFile;
   testMap[uiInputFile] = uiOutputFile;
   }
@@ -1321,6 +1329,13 @@ bool cmQtAutoGenerators::GenerateUi(const std::string& realName,
  );
   if (this->GenerateAll || !success || sourceNewerThanUi >= 0)
 {
+// make sure the directory for the resulting ui file exists
+std::string uiDir = uiBuildFile.substr(0, uiBuildFile.rfind('/'));
+if (!cmsys::SystemTools::FileExists(uiDir.c_str(), false))
+  {
+  cmsys::SystemTools::MakeDirectory(uiDir.c_str());
+  }
+
 std::string msg = "Generating ";
 msg += uiOutputFile;
 cmSystemTools::MakefileColorEcho(cmsysTerminal_Color_ForegroundBlue
-- 
2.8.0.rc3

-- 

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]

Am 19.04.2016 um 19:28 schrieb Brad King:

On 04/19/2016 11:09 AM, Sebastian Holtermann wrote:

https://cmake.org/Bug/view.php?id=12873
https://cmake.org/Bug/view.php?id=16068

They introduce
- same name collision checks during moc/qrc/ui generation
- moc/qrc generation in subdirectories to support
sources with the name in different subdirectories
- A test for equally named sources in different subdirectories

Please review.


Thanks!  I've applied them locally and merged the cleanup/refactoring
commits to 'next' for testing first.  Once those test cleanly I'll move
on to the rest.



Ok, thanks.

-Sebastian


--

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

[Brad King]

On 04/19/2016 11:09 AM, Sebastian Holtermann wrote:
> https://cmake.org/Bug/view.php?id=12873
> https://cmake.org/Bug/view.php?id=16068
> 
> They introduce
> - same name collision checks during moc/qrc/ui generation
> - moc/qrc generation in subdirectories to support
>sources with the name in different subdirectories
> - A test for equally named sources in different subdirectories
> 
> Please review.

Thanks!  I've applied them locally and merged the cleanup/refactoring
commits to 'next' for testing first.  Once those test cleanly I'll move
on to the rest.

-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