On 8/12/2016 10:36 AM, Wayne Stambaugh wrote: > On 8/12/2016 10:10 AM, Maciej Sumiński wrote: >> On 08/11/2016 05:37 PM, Wayne Stambaugh wrote: >>> Congratulations on everyone who made the is happen. This is a nice >>> feature for KiCad and hopefully users will find it useful. >>> >>> There are a few things will need to addressed that I missed when I >>> initially evaluated the code. >>> >>> Maybe I missed it but I do not see any indication that the ngspice >>> headers and libraries are detected during cmake configuration (see >>> attachment). This is a major no-no. All external dependencies must be >>> found during configuration. As a developer, there are few things that >>> make kick a software package to the curb faster than getting 3/4 of way >>> through a 30+ minute build only have the build fail because there is a >>> missing dependency header or library. This should fail during >>> configuration to inform the developer that there are missing >>> dependencies. Please write a FindNgSpice.cmake file. I know I've said >>> this in the past but I will reiterate it, if you add a new dependency to >>> KiCad you *must* add the configuration code to ensure that all of the >>> required headers and/or libraries can be found before a valid >>> configuration can be completed. >>> >>> Also, please update the Documentation/development/compiling.md file with >>> the added optional ng-spice dependency. This is the official compiling >>> document that gets pushed to the kicad website so it needs to be kept up >>> to date. >> >> Hi Wayne, >> >> Sure, both issues are fixed in revisions 7025 & 7027. Again, I had a >> chance to test the patches only under Windows and Linux, so their status >> for OS X is currently unknown. >> >> Regards, >> Orson > > Hi Orson, > > I saw your commit. Thanks for addressing this issue in a timely mannor. > I have on minor comment. FindFoo.cmake files should always have an > option to override the default find location which should take > precedence over the default locations. Typically this is done with an > environment variable and a definition such as FOO_ROOT. This allows > developers to use development libraries without having to overwrite > their system default libraries. I know many of the find modules that > ship with cmake do not do this. We should not be repeating this bad habit.
Whoops! I forgot to mention, version testing is always a good idea. At some point it will be an issue and you will have to add support for it. > > Thanks, > > Wayne > >> >>> Thanks again everyone for your hard work. >>> >>> Cheers, >>> >>> Wayne >> >> > _______________________________________________ Mailing list: https://launchpad.net/~kicad-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp
