Code review comment for lp:~amir-mohammadkhani/kicad/ash

On 9/30/2010 7:14 PM, Amir Mohammadkhani-Aminabadi wrote:
>
>> Can we assume that in your previous email to this one, that your remark
>> about WZ_lib being platform specific is you shooting from the hip, since
>> you now are saying you meant the Kicad version of FindwxWidgets.cmake
>> file all along, which cannot be platform specific? And who are you
>> going to say did not adequately read your patch?
>
> I did check the CMAKE cache to figure out what paths are already looked up when working on the zlib issues. My fallacy was to blindly assume that CMAKE modules would not define variables on some platforms and not on others.
>
> All the time I did talk about the Kicad version of FindwxWidgets.cmake. I never implied something different. And if you take a look at the original FindwxWidgets.cmake from CMAKE in any recent version you will notice that the reference to zlib is much later down the file.
> To quote you:
>>> My words have meaning.
>
> Now please take the time and explain how this is not platform specific:
>
> #=====================================================================
> # WIN32_FIND_STYLE
> #=====================================================================
> IF(wxWidgets_FIND_STYLE STREQUAL "win32")
> # Useful common wx libs needed by almost all components.
> SET(wxWidgets_COMMON_LIBRARIES png tiff jpeg zlib regex expat)

This is platform specific and broken by design. The last time I looked,
the version of FindwxWidgets.cmake that ships with cmake still suffers
from this problem. Experienced autotools developers would immediately
recognized the problem. Test for features not platforms. The way this
should really work is to first look for wx-config in the standard paths.
 If wx-config is not found, fall back to the hand rolled solution for
Windows. Your probably thinking what's the difference. Should the
wxWidgets folks figure out a way to come up with a wx-config for
Windows, this problem goes away. It will also work on every other
platform where wxWidgets provides wx-config without any modification.
Many of the cmake find modules suffer from this problem. A lot of this
stems from Microsoft not having a well defined file system layout for
development. The Kicad version of FindZLIB should also be modified to
look for the wxWidgets version of zlib.h and the library file if the
standard zlib files cannot be found. Adding the wxWidget path the the
zlib header and library search paths after the default paths should
solve the problem.

Wayne

>
>
> And yes, I find it highly confrontational, borderline insulting if one of the highest ranking contributors to the project flat out denies the patch without studying it. This is not a way to attract new contributors. If you were some causal contributor it might be different, but the way you did it, putting all your weight behind it in the first post, shredding it can only mean you wanted to drown the patch as fast as possible and try to bully out someone using VS before he becomes a regular contributor.
>
> Back on topic:
> Finished up and tested a patch on cmake. Trying to get ahold of one of the cmake devs to figure out if thats the right place to fix it.
>
> So, hopefully soon, you can go all rainbow on those comments without causing problems on the windows side.....
>
> cheers
> amir