Merge lp:~amir-mohammadkhani/kicad/ash into lp:kicad/product

Proposed by Amir Mohammadkhani-Aminabadi
Status: Merged
Merge reported by: Amir Mohammadkhani-Aminabadi
Merged at revision: not available
Proposed branch: lp:~amir-mohammadkhani/kicad/ash
Merge into: lp:kicad/product
Diff against target: 0 lines
To merge this branch: bzr merge lp:~amir-mohammadkhani/kicad/ash
Reviewer Review Type Date Requested Status
Amir Mohammadkhani-Aminabadi (community) Approve
Marco Serantoni (community) Approve
Review via email: mp+36886@code.launchpad.net

Description of the change

Var. compile and cmake fixes for win/vs2010

To post a comment you must log in.
Revision history for this message
Dick Hollenbeck (dickelbeck) wrote :

On 09/28/2010 11:14 AM, Amir Mohammadkhani-Aminabadi wrote:
> Amir Mohammadkhani-Aminabadi has proposed merging lp:~amir-mohammadkhani/kicad/ash into lp:kicad.
>
> Requested reviews:
> kicad-testing-committers (kicad-testing-committers)
>
>
> Var. compile and cmake fixes for win/vs2010
>

I think we are reaching the point where we need a separate branch for
visual c++.

Although in this specific case it seems like a patch could be made for
CMake, not Kicad.

I have no interest in continuing to bend over for Visual C++.

Another option is to simply drop support for it, which would be my first
choice.

I say no to this patch.

Dick

Revision history for this message
Marco Serantoni (marco-serantoni) wrote :

I've nothing against this patch, just some tabs instead spaces, but i think could be corrected easly.

review: Approve
Revision history for this message
Dick Hollenbeck (dickelbeck) wrote :

The linux version of CMake is handling the multiline COMMENT ok.

from eeschema/CMakeFiles/eeschema.dir/build.make:

It is putting out multiple echo_color commands for each line in the comment. It would be nice to see what the problem is with the reported environment. I say its Microsoft's or CMake's problem.

Dick

../eeschema/cmp_library_lexer.h: ../CMakeModules/TokenList2DsnLexer.cmake
        $(CMAKE_COMMAND) -E cmake_progress_report /svn/kicad/testing.checkout/release/CMakeFiles $(CMAKE_PROGRESS_129)
        @$(CMAKE_COMMAND) -E cmake_echo_color --switch=$(COLOR) --blue --bold "TokenList2DsnLexer.cmake creating:"
        @$(CMAKE_COMMAND) -E cmake_echo_color --switch=$(COLOR) --blue --bold " /svn/kicad/testing.checkout/eeschema/cmp_library_lexer.h and"
        @$(CMAKE_COMMAND) -E cmake_echo_color --switch=$(COLOR) --blue --bold " /svn/kicad/testing.checkout/eeschema/cmp_library_keywords.cpp from"
        @$(CMAKE_COMMAND) -E cmake_echo_color --switch=$(COLOR) --blue --bold " /svn/kicad/testing.checkout/eeschema/cmp_library.keywords"
        cd /svn/kicad/testing.checkout/release/eeschema && /usr/bin/cmake -Denum=TLIB_T -DinputFile=/svn/kicad/testing.checkout/eeschema/cmp_library.keywords -DoutHeaderFile=/svn/kicad/testing.checkout/eeschema/cmp_library_lexer.h -DoutCppFile=/svn/kicad/testing.checkout/eeschema/cmp_library_keywords.cpp -P /svn/kicad/testing.checkout/CMakeModules/TokenList2DsnLexer.cmake

Revision history for this message
Amir Mohammadkhani-Aminabadi (amir-mohammadkhani) wrote :

The problem is not with CMAKE perse but with VS interpreting the second line as command - which I too think is rather stupid and cmake should catch that.

All I could dig up is a report from 2006 where someone else had a similar problem.

http://<email address hidden>/msg01630.html

Revision history for this message
Amir Mohammadkhani-Aminabadi (amir-mohammadkhani) wrote :

> I think we are reaching the point where we need a separate branch for
> visual c++.
>
> Although in this specific case it seems like a patch could be made for
> CMake, not Kicad.
>
> I have no interest in continuing to bend over for Visual C++.
>
> Another option is to simply drop support for it, which would be my first
> choice.

Sorry, but this seems a rather small minded approach. Simply drop the most popular dev. env for windows because it requires some small changes another platform does not?

The multiline COMMENT thing seems to be unique to VisualStudio, but all other bugs are >mistakes< made by the original committer.

$(CMAKE_COMMAND) with ( instead of { will not work since its blatantly wrong.

As for atan2 - one should specify which one to use regardless of compiler as there are multiple overloads in cmath!

And as for the ZLIB_LIBRARIES fix, why should one lookup the zlib library used by wx in a rather complicated and >non working< way when wx supplies that in a nice variable.

The superflous escape ("\n\n\Error") ? Thats also an oversight by the original commiter which I stumbled upon because VS warned me about it.

As for the wxT - did you ever try to compile it using Unicode on linux?

So in the end in my opinion dropping VS support would be a bad idea, more compilers means more crosschecks, means more bugs cought.

After merging this patch the tree should generate and compile fine with CMAKE and VS2010, I would guess any other compiler would need atleast this amount of custom work as well - which is, leaving out the non VS issues, rather small.

Revision history for this message
Amir Mohammadkhani-Aminabadi (amir-mohammadkhani) wrote :

> I've nothing against this patch, just some tabs instead spaces, but i think
> could be corrected easly.

D'oh! I am usually using tabs, but I had the impression that spaces are used instead of tabs so I made an effort to use spaces. Which files do need fixing?

Revision history for this message
Wayne Stambaugh (stambaughw) wrote :

> > I've nothing against this patch, just some tabs instead spaces, but i think
> > could be corrected easly.
>
> D'oh! I am usually using tabs, but I had the impression that spaces are used
> instead of tabs so I made an effort to use spaces. Which files do need fixing?

Spaces are the policy for all Kicad source. I believe Marco meant that he found tabs in your patch that need to be cleaned up.

lp:~amir-mohammadkhani/kicad/ash updated
2505. By ash <ash@router19>

Tabs to spaces

Revision history for this message
Amir Mohammadkhani-Aminabadi (amir-mohammadkhani) wrote :

> Spaces are the policy for all Kicad source. I believe Marco meant that he
> found tabs in your patch that need to be cleaned up.

Thanks for clearing that up.
Please see r2505 - hopefully caught them all.

Revision history for this message
Dick Hollenbeck (dickelbeck) wrote :

http://cmake.org/gitweb?p=cmake.git;a=blob;f=Source/cmVisualStudio10TargetGenerator.cxx;h=8dfafffbbc4ef6cfdb4d42bb700335f791a8f278;hb=HEAD

See line 350 in the above link and compare that to your generated Microsoft project file. Apparently the "Message" tag is the problem. See if you can hand edit the project file to create something acceptable to your compiler.

Report those required changes and let's see if we can provide a patch to CMake.

I have no objections to anything in your patch other than the CMakeLists.txt COMMENT changes. This looks like a small issue to you, but it is the summation of all the cumulative Visual C++ crap that I object to. You cannot see that summation in 2 days of involvement with the project, so your comment about this being a small minded assessment will be ignored, for now.

I have no problem dropping support for Visual C++ if this continues. Mingw32 is available for Windows compiles.

Revision history for this message
Amir Mohammadkhani-Aminabadi (amir-mohammadkhani) wrote :

So far we have 1 vote for and 1 against.

Dick: In your first reply it seems you did not even read past "Changes for VC" since you wanted to trash the whole patch without even going into detail about >real non VC issues< that needed to be fixed.

I know you have no problem dropping VC since you are not using it. Mingw32 is not a suitable replacement for something like VS. You might be trashing MS software left and right, but VS is one of the best dev. envs there are, and the VC2010 compiler has gone a long way to be standard conforming.

To contribute this patch I had to learn launchpad, bazaar and cmake. Installing wxWidgets is as far as I go wasting my HDD because unlike native windows programs, things like wxWidgets and Mingw32 need to installed into the drives root to not cause problems down the road.

review: Approve
Revision history for this message
Wayne Stambaugh (stambaughw) wrote :
Download full text (3.4 KiB)

On 9/29/2010 10:24 AM, Amir Mohammadkhani-Aminabadi wrote:
> Review: Approve
> So far we have 1 vote for and 1 against.
>
>
> Dick: In your first reply it seems you did not even read past "Changes for VC" since you wanted to trash the whole patch without even going into detail about >real non VC issues< that needed to be fixed.

Amir,

Tread carefully here. Dick's assessment is correct about constantly tweaking
the source and build files to work correctly on Visual Studio. It has been an
on going struggle. I know. I'm the one who did a lot of the original work to
get it to build properly on VS2005. I have since become been less and less
enthused about keeping up with that effort due to time constraints. I
performed this task in hope that it would attract more developers from the
Windows world to possibly pitch in as I do not used VS personally for Kicad
development. I believe my effort has mostly been in vein. I do not believe
any of the regular contributors use VS. Otherwise, these build issues would
have been fixed a long time ago. Attacking one of the lead Kicad developers is
not the way to get your patch submitted or make it likely that you will be
extended the privilege of being asked to become a Kicad developer. Your best
approach would have been to acknowledge that there are differences between VC++
and GCC and volunteered to take the lead on keeping building Kicad on VS up to
date. Your efforts would have been taken much more seriously.

>
> I know you have no problem dropping VC since you are not using it. Mingw32 is not a suitable replacement for something like VS. You might be trashing MS software left and right, but VS is one of the best dev. envs there are, and the VC2010 compiler has gone a long way to be standard conforming.

This is merely your opinion. I have been coding for 25 years. Long before
IDEs existed. I have used VS 2.0, 4.2, 6.0, 2003, and 2005 and I can tell you
that they are not substitutes for an experienced developer. I started out
using command line build tools and I still use them. I see no advantage to an
IDE over my current Windows development environment (MinGW32/MSys). Since I
have used both over a long period of time, I think I am qualified to speak
intelligently about it. If you are comfortable using VS2010, by all means use
it. But making statements about the suitability of other development
environment on any platform is counterproductive. Developer should always use
what they are comfortable with.

>
>
> To contribute this patch I had to learn launchpad, bazaar and cmake. Installing wxWidgets is as far as I go wasting my HDD because unlike native windows programs, things like wxWidgets and Mingw32 need to installed into the drives root to not cause problems down the road.

Indeed. You are now a more qualified developer than you were when you started
down this path. That is good thing. You should also be thankful that all
these fine pieces of software are freely available for you to learn. Unless
you are using an illegal copy VS, you cannot say the same thing about it. As
for you HDD space, VS is not exactly light weight. My version of VS 2005
install weighs in at 2G and...

Read more...

Revision history for this message
Dick Hollenbeck (dickelbeck) wrote :

On 09/29/2010 09:24 AM, Amir Mohammadkhani-Aminabadi wrote:
> Review: Approve
> So far we have 1 vote for and 1 against.
>
>
> Dick: In your first reply it seems you did not even read past "Changes for VC" since you wanted to trash the whole patch without even going into detail about >real non VC issues< that needed to be fixed.
>

My last reply is more important.

I committed the pieces of your patch that I thought appropriate in
testing revision 2509. Your edits to basicframe.cpp would not compile
with g++ so I rewrote all that code.

Read my last reply and HAND EDIT your project file as an experimental
aid in determining what CMake needs to generate in order to create an
acceptable project file contain the 3 lines of comments.

CMake folks can take a patch and build a windows nightly download for you.

Your hard drive does not have to get filled up with CMake source files
if you do not want.

This will tells us if the bug is in CMake or in Visual C++.

It is not in CMakeLists.txt

Dick

Revision history for this message
Amir Mohammadkhani-Aminabadi (amir-mohammadkhani) wrote :
Download full text (6.7 KiB)

Hi Wayne,

thanks for going into detail about this.

> Tread carefully here. Dick's assessment is correct about constantly tweaking
> the source and build files to work correctly on Visual Studio. It has been an
> on going struggle. I know. I'm the one who did a lot of the original work to
> get it to build properly on VS2005. I have since become been less and less
> enthused about keeping up with that effort due to time constraints. I
> performed this task in hope that it would attract more developers from the
> Windows world to possibly pitch in as I do not used VS personally for Kicad
> development. I believe my effort has mostly been in vein. I do not believe
> any of the regular contributors use VS.

I do develop software professionally for over 14 years. Having worked with numerous environments and compilers during the time and having done a great deal of porting as well I surely acknowledge the point that each compiler/env needs certain adjustments.

But I do not think your work has been in vein.

For me, one of the main drawbacks on kicad are usability issues and how files are distributed. I don't feel comfortable downloading executable files from a site not directly associated with kicad, hosted on a completely different domain which might be linked from a kicad wiki page that can be edited by everyone. Without a way to compile kicad yourself you are stuck with a version that might be half a year old - missing essential bugfixes and features.

I did use kicad more than 1.5 years ago, and I think I did compile it myself back then. I switched back to eagle tho, because back then using kicad on an english windows with a german keyboard selected meant a lot of frustration inside kicad. Even now there are many little bugs in the way the GUI works that drive me crazy using the program.

But this time I decided to fix the bugs instead of leaving kicad for another solution again.

Add to the point, most people probably don't even know that kicad compiles using MSVC and the
alternative, setting up a MingW env. sounds painful.
All the documentation on the wiki refers to MingW! For me as Win user on the Desktop MingW means a lot of bending over and trashing the root drive with additional directories.

> Otherwise, these build issues would
> have been fixed a long time ago. Attacking one of the lead Kicad developers
> is
> not the way to get your patch submitted or make it likely that you will be
> extended the privilege of being asked to become a Kicad developer. Your best
> approach would have been to acknowledge that there are differences between
> VC++
> and GCC and volunteered to take the lead on keeping building Kicad on VS up to
> date. Your efforts would have been taken much more seriously.

I agree completely. But right out refusing a patch in what at least to me seems to be a purely ideologically driven response that comes across as rather arrogant isn't a way to treat someone who spent the better part of his free weekend to fix things for free.

I'd have had no problem with a response that was based on actually taking the time to study the patch, because as said, there are fixes for general bugs as well.

> This is merel...

Read more...

Revision history for this message
Amir Mohammadkhani-Aminabadi (amir-mohammadkhani) wrote :

Dick,

thanks for adding those changes.

Also, would you please add the following changes as well:

kicad/minizip/CMakeLists.txt

Since the wxWidget module of cmake already does save the path to the library used in a variable, it is better to use that variable instead of trying to look it up again via find.

Also please merge in at least the change in line 222 in pcbnew/CMakeLists.txt as that is a typo mistake as well. At that place curly brackets should be used instead of round ones.

As for the comment fix, yes cmake is the right place to fix it. But at the same time, what is gained by spreading them out over multiple lines - those output lines are quite superfluous and most likely nobody reads them.

Revision history for this message
Amir Mohammadkhani-Aminabadi (amir-mohammadkhani) wrote :

I am now trying to merge back the changes from the testing tree into mine and hope I do not make this discussion disappear in the process.

lp:~amir-mohammadkhani/kicad/ash updated
2506. By ash <ash@router19>

merged testing tree back in

Revision history for this message
Dick Hollenbeck (dickelbeck) wrote :

The CMake variable WX_zlib is a mystery. I cannot find it anywhere.

In what file and what version of that file do see this variable being introduced?

i.e.

a) filename and directory?
b) version of that file?

Revision history for this message
Amir Mohammadkhani-Aminabadi (amir-mohammadkhani) wrote :

Dick,

about WX_zlib - it seems I haven't been careful enough here. Looking through FindwxWidgets.cmake it seems this variable introduced on the Win side of the execution path only. AFAIK its introduced in line 190 and assigned generically later in that code path.

I do need to finish some work first, but I will take a look in the evening and see if I can rectify the problem in a way suitable for both platforms.

If you guys (JP Charras, Dick Hollenbeck and Wayne Stambaugh) could get together and decide if VS is going to be deprecated in the future or not, that would be great.

This would help me decide if its worth the effort to setup up a linux dev. env as well to cross check any changes on both systems - as well as updating the Wiki to reflect the possibility of building kicad on VS (Express I will test)

regards
amir

Revision history for this message
Dick Hollenbeck (dickelbeck) wrote :

On 09/30/2010 10:57 AM, Amir Mohammadkhani-Aminabadi wrote:
> Dick,
>
> about WX_zlib - it seems I haven't been careful enough here. Looking through FindwxWidgets.cmake it seems this variable introduced on the Win side of the execution path only. AFAIK its introduced in line 190 and assigned generically later in that code path.
>

I asked for a path and a version, as clearly as I could.

I'm sorry to say that this is becoming a write only conversation.

FindwxWidgets.cmake

is only a path.

There is one in CMakeModules, supplied with Kicad.

There is one in CMake.

My words have meaning.

I will not agree to splitting the COMMENT lines. Get CMake fixed, I
offered to help you with that. But now I rescind, the offer.

Suggestion: take it up with the CMake folks, send them a patch of your
project file, generated between one that works, one that does not. Or
take it up with Microsoft.

Dick

Revision history for this message
Wayne Stambaugh (stambaughw) wrote :

On 9/30/2010 11:57 AM, Amir Mohammadkhani-Aminabadi wrote:
> Dick,
>
> about WX_zlib - it seems I haven't been careful enough here. Looking through FindwxWidgets.cmake it seems this variable introduced on the Win side of the execution path only. AFAIK its introduced in line 190 and assigned generically later in that code path.
>
> I do need to finish some work first, but I will take a look in the evening and see if I can rectify the problem in a way suitable for both platforms.
>
> If you guys (JP Charras, Dick Hollenbeck and Wayne Stambaugh) could get together and decide if VS is going to be deprecated in the future or not, that would be great.

I have no plans to deprecate supporting VS or any other build environment in
the future. As long as someone is willing to maintain any build environment
that CMake supports and it isn't too disruptive with the known working build
environments, I have no objections.

Wayne

>
> This would help me decide if its worth the effort to setup up a linux dev. env as well to cross check any changes on both systems - as well as updating the Wiki to reflect the possibility of building kicad on VS (Express I will test)
>
> regards
> amir

Revision history for this message
Amir Mohammadkhani-Aminabadi (amir-mohammadkhani) wrote :

> On 09/30/2010 10:57 AM, Amir Mohammadkhani-Aminabadi wrote:
> > Dick,
> >
> > about WX_zlib - it seems I haven't been careful enough here. Looking through
> FindwxWidgets.cmake it seems this variable introduced on the Win side of the
> execution path only. AFAIK its introduced in line 190 and assigned generically
> later in that code path.
> >
>
> I asked for a path and a version, as clearly as I could.
>
>
> I'm sorry to say that this is becoming a write only conversation.
>
> FindwxWidgets.cmake
>
> is only a path.
>
> There is one in CMakeModules, supplied with Kicad.
>
> There is one in CMake.
>
> My words have meaning.

If you would have taken the time you took writing another post in a rather confrontational tone and would have taken a look at the FindwxWidgets.cmake in the kicad branch, or for that matter in the branch you're commenting on, you would have noticed that the only reference to zlib is on line 190 and that most likely I meant that file.

Take a look here - line 190 and following:
http://bazaar.launchpad.net/~kicad-testing-committers/kicad/testing/annotate/2512/CMakeModules/FindwxWidgets.cmake

But really, that point is mood, as I have already stated that my patch is not really a solution for both platforms as that variable is only assigned on windows.

> I will not agree to splitting the COMMENT lines. Get CMake fixed, I
> offered to help you with that. But now I rescind, the offer.
>
> Suggestion: take it up with the CMake folks, send them a patch of your
> project file, generated between one that works, one that does not. Or
> take it up with Microsoft.

Yes you are right, I will take matters to cmake to get it fixed at the proper place.

cheers
amir

Revision history for this message
Dick Hollenbeck (dickelbeck) wrote :

On 09/30/2010 01:34 PM, Amir Mohammadkhani-Aminabadi wrote:
>> On 09/30/2010 10:57 AM, Amir Mohammadkhani-Aminabadi wrote:
>>
>>> Dick,
>>>
>>> about WX_zlib - it seems I haven't been careful enough here. Looking through
>>>
>> FindwxWidgets.cmake it seems this variable introduced on the Win side of the
>> execution path only. AFAIK its introduced in line 190 and assigned generically
>> later in that code path.
>>
>>>
>> I asked for a path and a version, as clearly as I could.
>>
>>
>> I'm sorry to say that this is becoming a write only conversation.
>>
>> FindwxWidgets.cmake
>>
>> is only a path.
>>
>> There is one in CMakeModules, supplied with Kicad.
>>
>> There is one in CMake.
>>
>> My words have meaning.
>>
> If you would have taken the time you took writing another post in a rather confrontational tone and would have taken a look at the FindwxWidgets.cmake in the kicad branch, or for that matter in the branch you're commenting on, you would have noticed that the only reference to zlib is on line 190 and that most likely I meant that file.
>
> Take a look here - line 190 and following:
> http://bazaar.launchpad.net/~kicad-testing-committers/kicad/testing/annotate/2512/CMakeModules/FindwxWidgets.cmake
>
> But really, that point is mood, as I have already stated that my patch is not really a solution for both platforms as that variable is only assigned on windows.
>

Can we assume that English is not your first language?

You have not seen me confrontational.

However I've made a note that you think I am confrontational and small
minded.

I am now going to put you into my spam folder.

Dick

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?

> cheers
> amir
>
>

Revision history for this message
Amir Mohammadkhani-Aminabadi (amir-mohammadkhani) 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)

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

Revision history for this message
Amir Mohammadkhani-Aminabadi (amir-mohammadkhani) wrote :

Here is the relevant portion of my cmake cache file.

Again, my mistake was to assume that those variables are filled on all platforms and thus if we want to fall back to the zlib used by wxWidgets we might as well just take the variable.

//Cleared.
WX_zlib:FILEPATH=C:/wxWidgets-2.9.1/lib/vc_lib/wxzlib.lib

//Cleared.
WX_zlibd:FILEPATH=WX_zlibd-NOTFOUND

//path to store renamed .xpm files for compilation
XPM_CPP_PATH:PATH=C:/temp/kicad/ash/bitmaps/auto_renamed_to_cpp

//Path to a file.
ZLIB_INCLUDE_DIR:PATH=C:/wxWidgets-2.9.1/src/zlib

//Path to a library.
ZLIB_LIBRARY:FILEPATH=ZLIB_LIBRARY-NOTFOUND

Revision history for this message
Wayne Stambaugh (stambaughw) wrote :
Download full text (3.3 KiB)

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

Read more...

Revision history for this message
Amir Mohammadkhani-Aminabadi (amir-mohammadkhani) wrote :

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

Hi Wayne,

yes, even the recent version of FindwxWidgets.cmake that comes with CMAKE suffers from the same problem. Being new to CMAKE I truly believed there wouldnt be variables that should be filled on all platforms but are not, thus I wholeheartedly trusted the cache file and assumed that variable will be filled on all platforms.

Currently FindZLIB tries to find zlib in very specific locations. If that fails, later on CMakeList.txt in kicad\minizip will try to fall back on the zlib supplied by wxWidget.

It does that with a very crude find which is quite specific to *nix.

As far as I know CMAKE does not truly support recursive search for FIND* thus one has to know where to look or use workarounds like */*/*/*.

That was my first attempt, but after seeing wx_ZLIB in the cache file I thought I should be using that one.

cheers
amir

Revision history for this message
Amir Mohammadkhani-Aminabadi (amir-mohammadkhani) wrote :

Reported to CMAKE and supplied a fix.

See http://www.cmake.org/Bug/view.php?id=11283

lp:~amir-mohammadkhani/kicad/ash updated
2507. By ash <ash@router19>

remerged testing + reverted COMMENT to parent HEAD

Revision history for this message
Dick Hollenbeck (dickelbeck) wrote :

On 09/30/2010 09:33 PM, Amir Mohammadkhani-Aminabadi wrote:
> Reported to CMAKE and supplied a fix.
>
> See http://www.cmake.org/Bug/view.php?id=11283
>

Nice!

That should be acceptable to them, or at least they can fully understand
the issue and offer their own fix.

You did good.

Dick

Revision history for this message
Dick Hollenbeck (dickelbeck) wrote :

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

I *did* want to drown the patch as a unit.

Some parts however I did commit, and had to do that by hand.

Perhaps I did let my frustration with Microsoft issues ruining the time
I spent on the CMakeList.txt editing affect my response. Those COMMENT
lines were put in there by me, at some investment of time to make them
look nice. Then here comes Microsoft crapping on me again, I go ballistic.

I am human:

1) I am permanently wired to resent anything that will cause *me* to do
anything to help Microsoft accomplish anything, including making you
happy with Visual C++. Changing the CMakeLists.txt file sacrifices my
investment in that effort. My time is money, and I see this as me
spending money helping Microsoft win. It ain't going to happen.

2) I resent it when folks waste my time, this is anybody, but especially
Microsoft. You have no idea what Microsoft has cost me over the years.

This is my lens of the issue.

I did not mean to insult you, and I apologize for not simply sending you
a more detailed explanation of my response. You are correct, it was not
particularly welcoming.

But again, if the Microsoft crap keeps falling on my head, I will have
to do something to eliminate that. Others may worship them, I avoid them.

Dick

Preview Diff

Empty