Re: [cmake-developers] [PATCH] Fixed Borland linker issue when used with Ninja generator

2015-06-18 Thread Brad King
On 06/18/2015 03:05 PM, James Johnston wrote:
> See updated attached patch.

Thanks, applied:

 Embarcadero: Run at most one linker invocation at a time
 http://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=078b60f0

I moved most of the explanation from comments to the commit message
and left a minimal comment instead.

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] [PATCH] Fixed Borland linker issue when used with Ninja generator

2015-06-18 Thread James Johnston
See updated attached patch.  I've retested it to ensure the VTK 5.4.2 build
I've been working with still builds correctly - and it still does.

Thank you for considering it.

-James

> -Original Message-
> From: Brad King [mailto:brad.k...@kitware.com]
>
> > +set(CMAKE_JOB_POOL_LINK BCC32LinkPool)
> 
> Hmm.  What if the user sets this too?  Perhaps we should allow
> users/projects to take control of this setting by making the whole block
> conditional:
> 
>   if(NOT DEFINED CMAKE_JOB_POOL_LINK)

Fair enough.  Change made.  I can't think of a real-world reason why someone
would want to do this (given the buggy behavior that will result), but I
suppose it's better not to step in the way.  (The only thing I could think
of would be if you wanted both compiler and linker to use the SAME job pool,
which would then have to be restricted to 1.  At that point, there's no
reason to using Ninja in the first place...)

> 
> > +get_property(_EMB_TMP_POOLS GLOBAL PROPERTY JOB_POOLS)
> list(APPEND
> > +_EMB_TMP_POOLS BCC32LinkPool=1) set_property(GLOBAL PROPERTY
> > +JOB_POOLS ${_EMB_TMP_POOLS})
> 
> This can be just
> 
>  set_property(GLOBAL APPEND PROPERTY JOB_POOLS BCC32LinkPool=1)

Oops, I overlooked that!  Much cleaner now, thanks :)



0001-Work-around-Borland-linker-issue-when-used-with-Ninj.patch
Description: Binary data
-- 

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] [PATCH] Fixed Borland linker issue when used with Ninja generator

2015-06-18 Thread Brad King
On 06/18/2015 01:41 PM, James Johnston wrote:
> 3.  Patch Windows-Embarcadero.cmake, as I propose here.

I think that makes sense.

> +set(CMAKE_JOB_POOL_LINK BCC32LinkPool)

Hmm.  What if the user sets this too?  Perhaps we should allow
users/projects to take control of this setting by making the
whole block conditional:

  if(NOT DEFINED CMAKE_JOB_POOL_LINK)

> +get_property(_EMB_TMP_POOLS GLOBAL PROPERTY JOB_POOLS)
> +list(APPEND _EMB_TMP_POOLS BCC32LinkPool=1)
> +set_property(GLOBAL PROPERTY JOB_POOLS ${_EMB_TMP_POOLS})

This can be just

 set_property(GLOBAL APPEND PROPERTY JOB_POOLS BCC32LinkPool=1)

Please try out all the above and send a revised patch.

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


[cmake-developers] [PATCH] Fixed Borland linker issue when used with Ninja generator

2015-06-18 Thread James Johnston
Hi,

The attached patch is in regards to issue #15620 in mantis bug tracker
("Ninja generator triggers race condition in Borland bcc32 linker causing
intermittent build failure").  Thanks to Brad King's suggestion, I think
using job pools is a lot easier than trying my proposed workaround (which I
realized would probably involve more disruptive code changes).  (I'm new to
both CMake/Ninja, so wasn't aware of this option).

I've tested with building VTK 5.4.2.  Before the patch, 5 out of 5 builds
failed (sooner or later, in a big project like VTK, the race condition would
trash the build).  After the patch, 5 out of 5 builds work without error.

Because of this issue with the linker itself, I can't think of any reason
why anybody would want to change the default settings I put in
Modules/Platform/Windows-Embarcadero.cmake with this patch.  You simply
*must* restrict the linker to one instance, without exception, or you'll
have problems due to the bcc32 linker design flaws I've outlined in the bug
tracker and patch comments.  I was only able to think of three solutions to
this problem:

1.  Modify each upstream CMake project (e.g. VTK) itself to set the Ninja
job pool in the Borland case (not a good option).
2.  Provide a toolchain file that sets up the job pool.  Awkward, since you
have to do this EVERY TIME you use the Ninja generator /w bcc32 or it WILL
NOT WORK.
3.  Patch Windows-Embarcadero.cmake, as I propose here.

(Since JOB_POOLS is a global property and not a variable, I couldn't find a
way to simply pass this as a parameter to cmake.exe - not that that would be
an ideal solution anyway.)

Best regards,

James Johnston



0001-Work-around-Borland-linker-issue-when-used-with-Ninj.patch
Description: Binary data
-- 

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