Re: [cmake-developers] Using CMake generated ninja file as a subninja file

[Brad King]

On 05/17/2016 01:42 PM, Nicolas Desprès wrote:
> That's great! The code is way cleaner this way. I have tested it with
> my generator and it works fine.

Great, thanks for trying it out.  We'll see how nightly testing goes tonight
but otherwise I think this topic is all set.  Thanks for implementing this!

-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] Using CMake generated ninja file as a subninja file

[Nicolas Desprès]

On Tue, May 17, 2016 at 4:23 PM, Brad King  wrote:

> On 05/13/2016 07:19 PM, Nicolas Desprès wrote:
> > Ok. See the new implementation
>
> Thanks.  I've rebased your topic on 'master' after the style transition
> commit and updated the style accordingly.  Then I split the topic up into
> more commits and simplified the prefix insertion logic a bit.
>
> I've merged the topic to 'next' for testing here:
>
>  Merge topic 'ninja-output-path-prefix' into next
>  https://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=48d6c426
>
> It consists of a series of minor cleanups/refactoring/fixes followed
> by the main two commits:
>
>  Ninja: Pass all build paths through a central method
>  https://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=038e7716
>
>  Ninja: Support embedding of CMake as subninja project
>  https://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=8a862a4d
>
> Please review the revised approach.
>

That's great! The code is way cleaner this way. I have tested it with my
generator and it works fine.


>
> > But the ExportImport and Plugin tests are likely to fail when used with
> > this new feature. I can't figure out right now how to add a test for this
> > without duplicating the Plugin and ExportImport test into
> RunCMake/Ninja/.
>
> This is still the case, but I think it can be tackled as a follow-up
> effort.  The cmGlobalNinjaGenerator::NinjaOutputPath method could
> just be hacked to skip "paths" starting in `-Wl,` (or even just `-`)
> to handle the specific corner case.
>

I did not figure out a better way to handle this corner case. I have no
strong requirements on this corner case at the moment, so I am ok to tackle
it later.

Thanks a lot for your help,
-Nico
-- 

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] Using CMake generated ninja file as a subninja file

[Brad King]

On 05/13/2016 07:19 PM, Nicolas Desprès wrote:
> Ok. See the new implementation

Thanks.  I've rebased your topic on 'master' after the style transition
commit and updated the style accordingly.  Then I split the topic up into
more commits and simplified the prefix insertion logic a bit.

I've merged the topic to 'next' for testing here:

 Merge topic 'ninja-output-path-prefix' into next
 https://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=48d6c426

It consists of a series of minor cleanups/refactoring/fixes followed
by the main two commits:

 Ninja: Pass all build paths through a central method
 https://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=038e7716

 Ninja: Support embedding of CMake as subninja project
 https://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=8a862a4d

Please review the revised approach.

> But the ExportImport and Plugin tests are likely to fail when used with
> this new feature. I can't figure out right now how to add a test for this
> without duplicating the Plugin and ExportImport test into RunCMake/Ninja/. 

This is still the case, but I think it can be tackled as a follow-up
effort.  The cmGlobalNinjaGenerator::NinjaOutputPath method could
just be hacked to skip "paths" starting in `-Wl,` (or even just `-`)
to handle the specific corner case.

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] Using CMake generated ninja file as a subninja file

[Brad King]

On 05/13/2016 03:25 PM, Brad King wrote:
> so for now please make your logic recognize this case and work around it.

Please split this into two commits to move hunks like this:

> +  this->TargetAll = this->NinjaOutputPath("all");
> +  this->CMakeCacheFile = this->NinjaOutputPath("CMakeCache.txt");

into a preceding commit that performs refactoring with no functional change.

Also, in hunks like this:

> -  std::string convPath = ng->Convert(path, cmOutputConverter::HOME_OUTPUT);
> +  std::string convPath = ng->Convert(path, cmOutputConverter::FULL, format);

please revise the logic so that *nothing changes* from the old logic
when no subninja prefix is set.  If this is going to break anything I'd
like it to be isolated to when this feature is used.  Consolidation of
the two code paths can be done later when we've seen the new feature in
use for a while and gained confidence in 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] Using CMake generated ninja file as a subninja file

[Brad King]

On 05/13/2016 12:13 PM, Nicolas Desprès wrote:
> with a value that looks like this: 
> "-Wl,-bundle_loader,/Users/polrop/Documents/cmake/_build 
> ninja/Tests/ExportImport/Export/testExe2"
> 
> lie on the caller site because the "if (li->IsPath)" in OutputLinkLibraries()
> should not be true for such a value.

The problem is that "IsPath" really means "quote this like a path on the 
command line".
The value above *does* need such quoting.  We cannot make the !IsPath code path
do the quoting because it is not expected to do so.  This is kind of a corner 
case
where the link item is a flag that contains a path that needs to be treated as a
path.  Major refactoring would be needed to encode that information structurally
so for now please make your logic recognize this case and work around 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] Using CMake generated ninja file as a subninja file

[Nicolas Desprès]

On Thu, May 12, 2016 at 10:07 PM, Brad King  wrote:

> On 05/12/2016 02:16 PM, Nicolas Desprès wrote:
> > All done. Thank you for taking a look.
> > https://github.com/nicolasdespres/CMake/commits/ninja-output-path-prefix
>
> The RunCMake.NinjaOutputPathPrefix test fails for me when I have
> the CMake source and build trees in a directory with a space in
> the path.  Also, please move these test cases over to the
> Tests/RunCMake/Ninja directory.  You should be able to just append
> the RunCMakeTest.cmake content.
>
>
I have fixed the bug when a space is in the path and I added a test case
when a space is in the contents of the CMAKE_NINJA_OUTPUT_PATH_PREFIX
variable.

I have 2 failing tests left that are not failing on master: Plugin and
ExportImport.

Both suffers from the same problem which comes from this part of my patch:
https://github.com/nicolasdespres/CMake/commit/1f880c04bcb8cf36cf40be7fa4ef65f9525ea63e#diff-80cd058f986b2b3d5cdafc48a091411eL134

cmLocalNinjaGenerator::ConvertToLinkReference gets called by
cmLocalGenerator::OutputLinkLibraries from here:
https://github.com/nicolasdespres/CMake/blob/1f880c04bcb8cf36cf40be7fa4ef65f9525ea63e/Source/cmLocalGenerator.cxx#L1672
with a value that looks like this:
"-Wl,-bundle_loader,/Users/polrop/Documents/cmake/_build
ninja/Tests/ExportImport/Export/testExe2"

The previous implementation of ConvertToLinkReference returned the string
unchanged because it think it is a relative path, hiding the bug that lie
on the caller site because the "if (li->IsPath)" in OutputLinkLibraries()
should not be true for such a value. I don't know how to fix this.

Cheers,

-- 
Nicolas Desprès
-- 

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] Using CMake generated ninja file as a subninja file

[Brad King]

On 05/12/2016 02:16 PM, Nicolas Desprès wrote:
> All done. Thank you for taking a look.
> https://github.com/nicolasdespres/CMake/commits/ninja-output-path-prefix

The RunCMake.NinjaOutputPathPrefix test fails for me when I have
the CMake source and build trees in a directory with a space in
the path.  Also, please move these test cases over to the
Tests/RunCMake/Ninja directory.  You should be able to just append
the RunCMakeTest.cmake content.

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] Using CMake generated ninja file as a subninja file

[Nicolas Desprès]

On Thu, May 12, 2016 at 3:16 PM, Brad King  wrote:

> On 05/12/2016 06:16 AM, Nicolas Desprès wrote:
> > Brad has just sent a notification email about an upcoming feature freeze.
> > Do you think we could have this feature merged before?
>
> I'll take a look.  Please rebase on 'master' to resolve conflicts
> and also reconcile with the ConvertToNinjaFolderRule logic that was
> added.  Also please squash your fixup commits.
>
>
All done. Thank you for taking a look.
https://github.com/nicolasdespres/CMake/commits/ninja-output-path-prefix

-- 
Nicolas Desprès
-- 

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] Using CMake generated ninja file as a subninja file

[Brad King]

On 05/12/2016 06:16 AM, Nicolas Desprès wrote:
> Brad has just sent a notification email about an upcoming feature freeze.
> Do you think we could have this feature merged before?

I'll take a look.  Please rebase on 'master' to resolve conflicts
and also reconcile with the ConvertToNinjaFolderRule logic that was
added.  Also please squash your fixup commits.

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] Using CMake generated ninja file as a subninja file

[Nicolas Desprès]

Hi Ben,

Brad has just sent a notification email about an upcoming feature freeze.
Do you think we could have this feature merged before?

Regards,
Nicolas

On Sun, Mar 20, 2016 at 1:47 PM, Nicolas Desprès 
wrote:

>
>
> On Wed, Mar 9, 2016 at 9:41 PM, Ben Boeckel 
> wrote:
>
>> On Tue, Mar 08, 2016 at 12:36:31 +0100, Nicolas Desprès wrote:
>> > Did you have a chance to review my patches?
>>
>> So I looked at it today, and it looks good overall. A few niggles:
>>
> Thanks
>
>>
>> +inline bool cmEndsWith(const std::string& str, const std::string& what)
>> +{
>> +  assert(str.size() >= what.size());
>>
>> Probably better to just "return false;" if this would fire. Better than
>> crashing if something in a release would have hit this.
>>
>>
> Ok.
>
>
>> +  return str.compare(str.size() - what.size(), what.size(), what) == 0;
>> +}
>> +
>> +inline void cmStripSuffixIfExists(std::string* str,
>> +  const std::string& suffix)
>> +{
>> +  assert(str != NULL);
>>
>> Why not just use a reference rather than a pointer?
>>
>
> I don't like to use non-const reference argument because the caller may
> not expect its variable to be modified since it not passed it by address.
> Anyway, reference argument are used in other places in the source code so
> it does not matter.
>
>
>>
>> +  if (cmEndsWith(*str, suffix))
>> +str->resize(str->size() - suffix.size());
>>
>> Missing braces.
>>
>
> Ok.
>
>
>>
>> -std::string cmGlobalNinjaGenerator::ConvertToNinjaPath(const
>> std::string& path)
>> +static void EnsureTrailingSlash(std::string* path)
>> +{
>> +  assert(path != NULL);
>>
>> Same thing: why not a reference?
>>
> Done.
>
>>
>> +  if (path->empty())
>> +return;
>> +  std::string::value_type last = (*path)[path->size()-1];
>> +#ifdef _WIN32
>> +  if (last != '\\')
>> +*path += '\\';
>> +#else
>> +  if (last != '/')
>> +*path += '/';
>> +#endif
>>
>> Missing braces in the if statements here.
>>
>>
> Ok.
>
>
>> -  cmGlobalNinjaGenerator::WriteDefault(os,
>> -   outputs,
>> -   "Make the all target the
>> default.");
>> +  if (!this->HasOutputPathPrefix())
>> +cmGlobalNinjaGenerator::WriteDefault(os,
>> + outputs,
>> + "Make the all target the
>> default.");
>>
>> Missing braces.
>>
> Ok.
>
>>
>> +void
>> +cmGlobalNinjaGenerator::StripNinjaOutputPathPrefixAsSuffix(std::string*
>> path)
>> +{
>> +  assert(path != NULL);
>>
>> More pointers :) .
>>
> Ok.
>
>>
>> +  if (path->empty())
>> +return;
>>
>> And braces.
>>
> Ok.
>
> The fixes are that commit:
>
> https://github.com/nicolasdespres/CMake/commit/3fa749a19847898fcbb5635a273b0d02707dd4bd
>
>
>> As for the tests:
>>
>> +  file(WRITE "${top_build_ninja}" "\
>> +subninja sub/build.ninja
>> +default sub/all
>> +")
>>
>> I think adding the (documented) bit:
>>
>> +rule RERUN
>> +  command = true
>> +  description = Testing regeneration
>> +  generator = 1
>> +
>> +build build.ninja: RERUN || sub/build.ninja
>> +  pool = console
>>
>> here and testing that if the CMakeLists.txt is touched that CMake reruns
>> would be worth it. It seems to work here, so keeping it working would be
>> great.
>>
>
> I guess that test should exist on the super-generator side. But it is
> probably safer to test it here too.
>
> The fix is in that commit:
>
> https://github.com/nicolasdespres/CMake/commit/13f341588bc6ee1cb0ec5dce8f44602f5d066ca9
>
> Tell me if you prefer I squash all the commits together before you review.
>
> Thanks,
>
> --
> Nicolas Desprès
>



-- 
Nicolas Desprès
-- 

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] Using CMake generated ninja file as a subninja file

[Nicolas Desprès]

On Wed, Mar 9, 2016 at 9:41 PM, Ben Boeckel  wrote:

> On Tue, Mar 08, 2016 at 12:36:31 +0100, Nicolas Desprès wrote:
> > Did you have a chance to review my patches?
>
> So I looked at it today, and it looks good overall. A few niggles:
>
Thanks

>
> +inline bool cmEndsWith(const std::string& str, const std::string& what)
> +{
> +  assert(str.size() >= what.size());
>
> Probably better to just "return false;" if this would fire. Better than
> crashing if something in a release would have hit this.
>
>
Ok.


> +  return str.compare(str.size() - what.size(), what.size(), what) == 0;
> +}
> +
> +inline void cmStripSuffixIfExists(std::string* str,
> +  const std::string& suffix)
> +{
> +  assert(str != NULL);
>
> Why not just use a reference rather than a pointer?
>

I don't like to use non-const reference argument because the caller may not
expect its variable to be modified since it not passed it by address.
Anyway, reference argument are used in other places in the source code so
it does not matter.


>
> +  if (cmEndsWith(*str, suffix))
> +str->resize(str->size() - suffix.size());
>
> Missing braces.
>

Ok.


>
> -std::string cmGlobalNinjaGenerator::ConvertToNinjaPath(const std::string&
> path)
> +static void EnsureTrailingSlash(std::string* path)
> +{
> +  assert(path != NULL);
>
> Same thing: why not a reference?
>
Done.

>
> +  if (path->empty())
> +return;
> +  std::string::value_type last = (*path)[path->size()-1];
> +#ifdef _WIN32
> +  if (last != '\\')
> +*path += '\\';
> +#else
> +  if (last != '/')
> +*path += '/';
> +#endif
>
> Missing braces in the if statements here.
>
>
Ok.


> -  cmGlobalNinjaGenerator::WriteDefault(os,
> -   outputs,
> -   "Make the all target the
> default.");
> +  if (!this->HasOutputPathPrefix())
> +cmGlobalNinjaGenerator::WriteDefault(os,
> + outputs,
> + "Make the all target the
> default.");
>
> Missing braces.
>
Ok.

>
> +void
> +cmGlobalNinjaGenerator::StripNinjaOutputPathPrefixAsSuffix(std::string*
> path)
> +{
> +  assert(path != NULL);
>
> More pointers :) .
>
Ok.

>
> +  if (path->empty())
> +return;
>
> And braces.
>
Ok.

The fixes are that commit:
https://github.com/nicolasdespres/CMake/commit/3fa749a19847898fcbb5635a273b0d02707dd4bd


> As for the tests:
>
> +  file(WRITE "${top_build_ninja}" "\
> +subninja sub/build.ninja
> +default sub/all
> +")
>
> I think adding the (documented) bit:
>
> +rule RERUN
> +  command = true
> +  description = Testing regeneration
> +  generator = 1
> +
> +build build.ninja: RERUN || sub/build.ninja
> +  pool = console
>
> here and testing that if the CMakeLists.txt is touched that CMake reruns
> would be worth it. It seems to work here, so keeping it working would be
> great.
>

I guess that test should exist on the super-generator side. But it is
probably safer to test it here too.

The fix is in that commit:
https://github.com/nicolasdespres/CMake/commit/13f341588bc6ee1cb0ec5dce8f44602f5d066ca9

Tell me if you prefer I squash all the commits together before you review.

Thanks,

-- 
Nicolas Desprès
-- 

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] Using CMake generated ninja file as a subninja file

[Ben Boeckel]

On Tue, Mar 08, 2016 at 12:36:31 +0100, Nicolas Desprès wrote:
> Did you have a chance to review my patches?

So I looked at it today, and it looks good overall. A few niggles:

+inline bool cmEndsWith(const std::string& str, const std::string& what)
+{
+  assert(str.size() >= what.size());

Probably better to just "return false;" if this would fire. Better than
crashing if something in a release would have hit this.

+  return str.compare(str.size() - what.size(), what.size(), what) == 0;
+}
+
+inline void cmStripSuffixIfExists(std::string* str,
+  const std::string& suffix)
+{
+  assert(str != NULL);

Why not just use a reference rather than a pointer?

+  if (cmEndsWith(*str, suffix))
+str->resize(str->size() - suffix.size());

Missing braces.

-std::string cmGlobalNinjaGenerator::ConvertToNinjaPath(const std::string& path)
+static void EnsureTrailingSlash(std::string* path)
+{
+  assert(path != NULL);

Same thing: why not a reference?

+  if (path->empty())
+return;
+  std::string::value_type last = (*path)[path->size()-1];
+#ifdef _WIN32
+  if (last != '\\')
+*path += '\\';
+#else
+  if (last != '/')
+*path += '/';
+#endif

Missing braces in the if statements here.

-  cmGlobalNinjaGenerator::WriteDefault(os,
-   outputs,
-   "Make the all target the default.");
+  if (!this->HasOutputPathPrefix())
+cmGlobalNinjaGenerator::WriteDefault(os,
+ outputs,
+ "Make the all target the default.");

Missing braces.

+void
+cmGlobalNinjaGenerator::StripNinjaOutputPathPrefixAsSuffix(std::string* path)
+{
+  assert(path != NULL);

More pointers :) .

+  if (path->empty())
+return;

And braces.

As for the tests:

+  file(WRITE "${top_build_ninja}" "\
+subninja sub/build.ninja
+default sub/all
+")

I think adding the (documented) bit:

+rule RERUN
+  command = true
+  description = Testing regeneration
+  generator = 1
+
+build build.ninja: RERUN || sub/build.ninja
+  pool = console

here and testing that if the CMakeLists.txt is touched that CMake reruns
would be worth it. It seems to work here, so keeping it working would be
great.

Thanks!

--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] Using CMake generated ninja file as a subninja file

[Ben Boeckel]

On Tue, Mar 08, 2016 at 12:36:31 +0100, Nicolas Desprès wrote:
> Did you have a chance to review my patches?

Sorry, not yet. I hope to have a peek at it today or tomorrow.

--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] Using CMake generated ninja file as a subninja file

[Nicolas Desprès]

Ben,

Did you have a chance to review my patches?

Cheers,
Nicolas

On Sunday, February 14, 2016, Nicolas Desprès 
wrote:

> Ben,
>
> I have pushed a complete version of the patch here:
> https://github.com/nicolasdespres/CMake/commits/ninja-output-path-prefix
>
> Since the last push, I have addressed the comments you made in your
> previous review (i.e. move some code to cmAlgorithm.h) and added a
> RunCMake.NinjaOutputPathPrefix tests.
>
> The 'default' statement is no longer generated when
> CMAKE_NINJA_OUTPUT_PATH_PREFIX is set because, unlike rules, "default"
> statement are not scoped. Thus, if the user does not include one in the top
> build.ninja only the default target of the subninja file was built. This
> was a surprising behaviour for me, since I expected to build all "leaf"
> output targets of the graph as usual when running ninja without argument.
>
> Let me know what you think about this change.
>
> Last note: some of the tests were failing on master (dec7d5c4de7870ec) on
> my machines (macbookpro and linux box), so I don't know if they are broken
> or not by my patch:
> 194 - CTestCoverageCollectGCOV (Failed)
> 393 - RunCMake.CommandLine (Failed)
> 401 - RunCMake.IfacePaths_INCLUDE_DIRECTORIES (Failed)
>
> Cheers,
> Nicolas
>
>
>
> On Fri, Jan 22, 2016 at 5:50 PM, Ben Boeckel  > wrote:
>
>> On Fri, Jan 22, 2016 at 17:34:03 +0100, Nicolas Desprès wrote:
>> > I have pushed a first draft of the patch here:
>> >
>> >
>> https://github.com/nicolasdespres/CMake/commit/67a4faad47378c07c97a29dd229d6f9fa500763b
>> >
>> > I have tested with a small project and it looks good.
>>
>> Looks like a good start to me as well. I added some comments on the
>> commit.
>>
>> > I would like to add some regression tests but I don't know from where to
>> > start.
>> >
>> > The principle would be to duplicate and modify existing tests so that
>> > CMAKE_NINJA_OUTPUT_PATH_PREFIX is set to "sub/" and the build directory
>> is
>> > changed from _build to _build/sub. A dummy build.ninja file containing
>> > "subninja sub/build.ninja" should be generated in _build/build.ninja and
>> > ninja should be called from _build instead of _build/sub.
>> >
>> > It would be great if you could point me some existing tests that could
>> > serve as a base to do the above.
>>
>> The RunCMake test infrastructure would probably be the best place to
>> start. It could be modified to do some of this work.
>>
>> Thanks,
>>
>> --Ben
>>
>
>
>
> --
> Nicolas Desprès
>


-- 
Nicolas Desprès
-- 

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] Using CMake generated ninja file as a subninja file

[Nicolas Desprès]

Ben,

I have pushed a complete version of the patch here:
https://github.com/nicolasdespres/CMake/commits/ninja-output-path-prefix

Since the last push, I have addressed the comments you made in your
previous review (i.e. move some code to cmAlgorithm.h) and added a
RunCMake.NinjaOutputPathPrefix tests.

The 'default' statement is no longer generated when
CMAKE_NINJA_OUTPUT_PATH_PREFIX is set because, unlike rules, "default"
statement are not scoped. Thus, if the user does not include one in the top
build.ninja only the default target of the subninja file was built. This
was a surprising behaviour for me, since I expected to build all "leaf"
output targets of the graph as usual when running ninja without argument.

Let me know what you think about this change.

Last note: some of the tests were failing on master (dec7d5c4de7870ec) on
my machines (macbookpro and linux box), so I don't know if they are broken
or not by my patch:
194 - CTestCoverageCollectGCOV (Failed)
393 - RunCMake.CommandLine (Failed)
401 - RunCMake.IfacePaths_INCLUDE_DIRECTORIES (Failed)

Cheers,
Nicolas



On Fri, Jan 22, 2016 at 5:50 PM, Ben Boeckel 
wrote:

> On Fri, Jan 22, 2016 at 17:34:03 +0100, Nicolas Desprès wrote:
> > I have pushed a first draft of the patch here:
> >
> >
> https://github.com/nicolasdespres/CMake/commit/67a4faad47378c07c97a29dd229d6f9fa500763b
> >
> > I have tested with a small project and it looks good.
>
> Looks like a good start to me as well. I added some comments on the
> commit.
>
> > I would like to add some regression tests but I don't know from where to
> > start.
> >
> > The principle would be to duplicate and modify existing tests so that
> > CMAKE_NINJA_OUTPUT_PATH_PREFIX is set to "sub/" and the build directory
> is
> > changed from _build to _build/sub. A dummy build.ninja file containing
> > "subninja sub/build.ninja" should be generated in _build/build.ninja and
> > ninja should be called from _build instead of _build/sub.
> >
> > It would be great if you could point me some existing tests that could
> > serve as a base to do the above.
>
> The RunCMake test infrastructure would probably be the best place to
> start. It could be modified to do some of this work.
>
> Thanks,
>
> --Ben
>



-- 
Nicolas Desprès
-- 

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] Using CMake generated ninja file as a subninja file

[Ben Boeckel]

On Fri, Jan 22, 2016 at 17:34:03 +0100, Nicolas Desprès wrote:
> I have pushed a first draft of the patch here:
> 
> https://github.com/nicolasdespres/CMake/commit/67a4faad47378c07c97a29dd229d6f9fa500763b
> 
> I have tested with a small project and it looks good.

Looks like a good start to me as well. I added some comments on the
commit.

> I would like to add some regression tests but I don't know from where to
> start.
> 
> The principle would be to duplicate and modify existing tests so that
> CMAKE_NINJA_OUTPUT_PATH_PREFIX is set to "sub/" and the build directory is
> changed from _build to _build/sub. A dummy build.ninja file containing
> "subninja sub/build.ninja" should be generated in _build/build.ninja and
> ninja should be called from _build instead of _build/sub.
> 
> It would be great if you could point me some existing tests that could
> serve as a base to do the above.

The RunCMake test infrastructure would probably be the best place to
start. It could be modified to do some of this work.

Thanks,

--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] Using CMake generated ninja file as a subninja file

[Nicolas Desprès]

Ben,

I have pushed a first draft of the patch here:

https://github.com/nicolasdespres/CMake/commit/67a4faad47378c07c97a29dd229d6f9fa500763b

I have tested with a small project and it looks good.

I would like to add some regression tests but I don't know from where to
start.

The principle would be to duplicate and modify existing tests so that
CMAKE_NINJA_OUTPUT_PATH_PREFIX is set to "sub/" and the build directory is
changed from _build to _build/sub. A dummy build.ninja file containing
"subninja sub/build.ninja" should be generated in _build/build.ninja and
ninja should be called from _build instead of _build/sub.

It would be great if you could point me some existing tests that could
serve as a base to do the above.

Cheers,
Nicolas

On Wed, Jan 6, 2016 at 7:46 PM, Nicolas Desprès 
wrote:

>
>
> On Wed, Jan 6, 2016 at 7:35 PM, Ben Boeckel 
> wrote:
>
>> On Wed, Jan 06, 2016 at 19:26:11 +0100, Nicolas Desprès wrote:
>> > Is that variable already available or a name suggestion? I cannot find
>> any
>> > reference to it neither in the source nor in the documentation.
>>
>> It's a suggestion.
>>
>
> Ok, works for me too.
>
>
>> > This is acceptable for me since I have a custom generator that would
>> write
>> > the top build.ninja containing "subninja sub/build.ninja". The end-user
>> is
>> > supposed to run ninja from the top directory not from the top/sub/
>> > directory.
>>
>> It looks like subclassing cmLocalNinjaGenerator to override some
>> path-related bits and it should work (though it has changed enough with
>> cmState that I can't tell what exactly without doing more digging).
>>
>> The hardest problem is going to be with testing. Every generate step
>> will also have to dump a build.ninja file out so that it can be properly
>> tested.
>>
>> Brad is back later this week and may have other ideas too.
>>
>> I will try to have a patch working on a helloworld project first and I
> will get back to you for the testing part. That would be the hardest part
> indeed.
>
> Thanks for your help.
>
> --
> Nicolas Desprès
>



-- 
Nicolas Desprès
-- 

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

[cmake-developers] Using CMake generated ninja file as a subninja file

[Nicolas Desprès]

Hello,

I would like to be able to use a build.ninja file generated by CMake as a
subninja file of a super build.ninja file generated by a custom generator.
This way I could include a CMake projects in a bigger one.

Currently, I call ninja recursively to build the CMake sub-project. But
this approach goes against ninja philosophy (one file to ease parallel job
handling because recursive makefile are considered harmful, etc...) and the
maintainers are not willing to accept patches to properly support it [1, 2].

Consider the following build directory layout:

top/build.ninja
top/sub/build.ninja

Where:
- top/build.ninja contains, among other things, "subninja sub/build.ninja"
and is generated by a custom generator
- "top/sub/build.ninja" is generated by CMake.

To properly work, build edge outputs in "top/sub/build.ninja" must be
relative to the "top/" directory.

Currently, it is impossible because to generate the "top/sub/build.ninja"
file CMake must be invoked like this: "cd top/sub && cmake -G Ninja
". It has no knowledge of the "top/" directory being special and
generates all the outputs relative to the "top/sub/" directory instead of
the "top/" directory.

For example, we should have this kind of generated build statements for an
helloworld project:

build sub/build.ninja: RERUN_CMAKE 
build sub/clean: CLEAN
build hello: C_EXECUTABLE_LINKER__hello sub/CMakeFiles/hello.dir/hello.c.o
build sub/all: phony sub/hello
default sub/all

To enable this type of generation, the "top/" directory must be passed to
CMake somehow. An environment variable or a CMake variable called
CMAKE_NINJA_TOP_BUILD_DIR would do the trick.

Note that such a change requires scoped rules which was introduced by ninja
1.6. [3]

I am willing to write the patch if you think there is a chance that it will
be accepted.

Best regards,

[1] https://github.com/ninja-build/ninja/pull/1079
[2] https://github.com/ninja-build/ninja/pull/1078
[3] https://ninja-build.org/manual.html#ref_scope

-- 
Nicolas Desprès
-- 

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] Using CMake generated ninja file as a subninja file

[Nicolas Desprès]

On Wed, Jan 6, 2016 at 6:25 PM, Ben Boeckel  wrote:

> On Wed, Jan 06, 2016 at 11:14:32 +0100, Nicolas Desprès wrote:
> > To enable this type of generation, the "top/" directory must be passed to
> > CMake somehow. An environment variable or a CMake variable called
> > CMAKE_NINJA_TOP_BUILD_DIR would do the trick.
>
> I think CMAKE_NINJA_OUTPUT_PATH_PREFIX would work fine (passed as a -D
> on the command line).
>

Is that variable already available or a name suggestion? I cannot find any
reference to it neither in the source nor in the documentation.


>
> Note that the ninja file CMake generates is then invalid except as a
> subninja file (since the build rules paths won't line up from its
> directory). I don't know how acceptable that would be. Maybe a separate
> Subninja generator which uses it and writes out a build.subninja file
> instead?
>
This is acceptable for me since I have a custom generator that would write
the top build.ninja containing "subninja sub/build.ninja". The end-user is
supposed to run ninja from the top directory not from the top/sub/
directory.

-- 
Nicolas Desprès
-- 

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] Using CMake generated ninja file as a subninja file

[Ben Boeckel]

On Wed, Jan 06, 2016 at 19:26:11 +0100, Nicolas Desprès wrote:
> Is that variable already available or a name suggestion? I cannot find any
> reference to it neither in the source nor in the documentation.

It's a suggestion.

> This is acceptable for me since I have a custom generator that would write
> the top build.ninja containing "subninja sub/build.ninja". The end-user is
> supposed to run ninja from the top directory not from the top/sub/
> directory.

It looks like subclassing cmLocalNinjaGenerator to override some
path-related bits and it should work (though it has changed enough with
cmState that I can't tell what exactly without doing more digging).

The hardest problem is going to be with testing. Every generate step
will also have to dump a build.ninja file out so that it can be properly
tested.

Brad is back later this week and may have other ideas too.

--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] Using CMake generated ninja file as a subninja file

[Nicolas Desprès]

On Wed, Jan 6, 2016 at 7:35 PM, Ben Boeckel  wrote:

> On Wed, Jan 06, 2016 at 19:26:11 +0100, Nicolas Desprès wrote:
> > Is that variable already available or a name suggestion? I cannot find
> any
> > reference to it neither in the source nor in the documentation.
>
> It's a suggestion.
>

Ok, works for me too.


> > This is acceptable for me since I have a custom generator that would
> write
> > the top build.ninja containing "subninja sub/build.ninja". The end-user
> is
> > supposed to run ninja from the top directory not from the top/sub/
> > directory.
>
> It looks like subclassing cmLocalNinjaGenerator to override some
> path-related bits and it should work (though it has changed enough with
> cmState that I can't tell what exactly without doing more digging).
>
> The hardest problem is going to be with testing. Every generate step
> will also have to dump a build.ninja file out so that it can be properly
> tested.
>
> Brad is back later this week and may have other ideas too.
>
> I will try to have a patch working on a helloworld project first and I
will get back to you for the testing part. That would be the hardest part
indeed.

Thanks for your help.

-- 
Nicolas Desprès
-- 

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] Using CMake generated ninja file as a subninja file

[Ben Boeckel]

On Wed, Jan 06, 2016 at 11:14:32 +0100, Nicolas Desprès wrote:
> To enable this type of generation, the "top/" directory must be passed to
> CMake somehow. An environment variable or a CMake variable called
> CMAKE_NINJA_TOP_BUILD_DIR would do the trick.

I think CMAKE_NINJA_OUTPUT_PATH_PREFIX would work fine (passed as a -D
on the command line).

Note that the ninja file CMake generates is then invalid except as a
subninja file (since the build rules paths won't line up from its
directory). I don't know how acceptable that would be. Maybe a separate
Subninja generator which uses it and writes out a build.subninja file
instead?

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