Hi Wayne, I understand the problems with linking when units are involved. This particular test executable defines GERBVIEW to satisfy the need to have a valid #define set in convert_to_biu.h. It doesn't actually use any unit code directly. TITLE_BLOCK is just a few strings in a class, but somehow one of the transitive includes ends up pulling enough stuff in that the define is needed.
I don't think this is caused by unit code in particular, normally that's pretty clear from the failure messages. Cheers, John On Fri, Oct 12, 2018 at 8:05 PM Wayne Stambaugh <stambau...@gmail.com> wrote: > > Hey John, > > Any code that has to be built multiple times to get the correct base > units scalar will most likely fail linking unless you only care about > testing a specific unit scalar such as PCBNEW or EESCHEMA in which case > you would need to include the appropriate libraries. You cannot make > any generic test such as the worksheet code which is unit scalar > dependent. You would have to create a separate test for each unit > scalar. In other words you would have to write a schematic, pcbnew, and > gerbview worksheet test since they each use a different units scalar. > > Cheers, > > Wayne > > On 10/9/2018 10:25 AM, John Beard wrote: > > Hi Wayne, > > > > No problems, I get it! > > > > It's not needed for the fuzzing stuff, it's actually a pre-cursor for > > a patch set to do with the dialog_page_settings dialog, but that is > > now probably on hold for a bit due to being unable to now include > > "class_drawpanel.h" from any common lib cpp. > > > > However, it's also needed to be able to use unit testing for many > > libcommon classes (TITLE_BLOCK is not the only one that ends up > > sucking in tons of dependencies), so if anyone has any ideas about > > what else is needed to get this to build, I'd appreciate pointers! > > > > Cheers, > > > > John > > On Tue, Oct 9, 2018 at 3:04 PM Wayne Stambaugh <stambau...@gmail.com> wrote: > >> > >> Hey John, > >> > >> I would like to help you out with this but I'm slammed at work and I'm > >> not going to be able to help out. Is this a patch a prerequisite for > >> your fuzzing patch? > >> > >> Cheers, > >> > >> Wayne > >> > >> On 10/8/2018 1:11 PM, John Beard wrote: > >>> Hi Wayne, > >>> > >>> I have no ideas why that would be, but the QA stuff does have a funny > >>> way of exposing strange link behaviours. In this case, we have: > >>> > >>> target_link_libraries( qa_common > >>> common > >>> polygon > >>> bitmaps > >>> ${Boost_UNIT_TEST_FRAMEWORK_LIBRARY} > >>> ${wxWidgets_LIBRARIES} # doesn't this provide wxcolourbase?? > >>> ) > >>> > >>> I have no explanation for why wxColourBase::FromString doesn't link > >>> here. Perhaps it's a similar reason for whatever prompted this being > >>> necessary for pcb_test_window: > >>> > >>> target_link_libraries( test_window > >>> polygon > >>> pnsrouter > >>> common > >>> pcbcommon > >>> bitmaps > >>> polygon > >>> pnsrouter > >>> common > >>> pcbcommon > >>> bitmaps > >>> polygon > >>> pnsrouter > >>> common > >>> pcbcommon > >>> bitmaps > >>> polygon > >>> pnsrouter > >>> common > >>> pcbcommon > >>> 3d-viewer > >>> bitmaps > >>> gal > >>> pcad2kicadpcb > >>> common > >>> pcbcommon > >>> ${GITHUB_PLUGIN_LIBRARIES} > >>> common > >>> pcbcommon > >>> ${Boost_FILESYSTEM_LIBRARY} > >>> ${Boost_SYSTEM_LIBRARY} > >>> ${Boost_UNIT_TEST_FRAMEWORK_LIBRARY} > >>> ${wxWidgets_LIBRARIES} > >>> ) > >>> > >>> Cheers, > >>> > >>> John > >>> On Mon, Oct 8, 2018 at 5:39 PM Wayne Stambaugh <stambau...@gmail.com> > >>> wrote: > >>>> > >>>> Hi John, > >>>> > >>>> I am getting the following build error on windows with this patch: > >>>> > >>>> C:/msys64/mingw32/bin/../lib/gcc/i686-w64-mingw32/7.3.0/../../../../i686-w64-mingw32/bin/ld.exe: > >>>> ../../common/libgal.a(color4d.cpp.obj):color4d.cpp:(.text+0x1c9): > >>>> undefined reference to `wxColourBase::FromString(wxString const&)' > >>>> C:/msys64/mingw32/bin/../lib/gcc/i686-w64-mingw32/7.3.0/../../../../i686-w64-mingw32/bin/ld.exe: > >>>> ../../common/libgal.a(color4d.cpp.obj):color4d.cpp:(.text+0x32e): > >>>> undefined reference to `wxColourBase::GetAsString(long) const' > >>>> C:/msys64/mingw32/bin/../lib/gcc/i686-w64-mingw32/7.3.0/../../../../i686-w64-mingw32/bin/ld.exe: > >>>> ../../common/libgal.a(color4d.cpp.obj):color4d.cpp:(.text+0x80c): > >>>> undefined reference to `wxColourBase::GetAsString(long) const' > >>>> C:/msys64/mingw32/bin/../lib/gcc/i686-w64-mingw32/7.3.0/../../../../i686-w64-mingw32/bin/ld.exe: > >>>> ../../common/libgal.a(gal_display_options.cpp.obj):gal_display_options.cpp:(.text+0x22b): > >>>> undefined reference to `wxConfigBase::Read(wxString const&, long*, long) > >>>> const' > >>>> C:/msys64/mingw32/bin/../lib/gcc/i686-w64-mingw32/7.3.0/../../../../i686-w64-mingw32/bin/ld.exe: > >>>> ../../common/libgal.a(gal_display_options.cpp.obj):gal_display_options.cpp:(.text+0x2b8): > >>>> undefined reference to `wxConfigBase::Read(wxString const&, long*, long) > >>>> const' > >>>> C:/msys64/mingw32/bin/../lib/gcc/i686-w64-mingw32/7.3.0/../../../../i686-w64-mingw32/bin/ld.exe: > >>>> ../../common/libgal.a(gal_display_options.cpp.obj):gal_display_options.cpp:(.text+0x347): > >>>> undefined reference to `wxConfigBase::Read(wxString const&, double*, > >>>> double) const' > >>>> C:/msys64/mingw32/bin/../lib/gcc/i686-w64-mingw32/7.3.0/../../../../i686-w64-mingw32/bin/ld.exe: > >>>> ../../common/libgal.a(gal_display_options.cpp.obj):gal_display_options.cpp:(.text+0x398): > >>>> undefined reference to `wxConfigBase::Read(wxString const&, double*, > >>>> double) const' > >>>> collect2.exe: error: ld returned 1 exit status > >>>> make[2]: *** [qa/common/CMakeFiles/qa_common.dir/build.make:217: > >>>> qa/common/qa_common.exe] Error 1 > >>>> make[1]: *** [CMakeFiles/Makefile2:3111: > >>>> qa/common/CMakeFiles/qa_common.dir/all] > >>>> > >>>> > >>>> > >>>> On 10/6/2018 3:48 PM, John Beard wrote: > >>>>> Hi Wayne, > >>>>> > >>>>> On Sat, Oct 6, 2018 at 5:29 PM Wayne Stambaugh <stambau...@gmail.com> > >>>>> wrote: > >>>>>> No problem. I will test your patch once you post it. > >>>>> > >>>>> Patch attached. Indeed, it was a missing include in the header that > >>>>> probably worked until now due to serendipitous include ordering in the > >>>>> existing CPPs. > >>>>> > >>>>> This actually illustrates one nice thing about having unit tests CPPs > >>>>> that include only a single KiCad header - they make sure that header > >>>>> and any transitive includes are compilable even when the test CPP only > >>>>> includes that one header (i.e. as recommended by the code guidelines: > >>>>> 6.2 Headers Without Unsatisfied Dependencies [1]). > >>>>> > >>>>> Another way this can improved is when you have a .cpp/.h pair for a > >>>>> self-contained class, the .cpp should include the corresponding .h > >>>>> first before any other include. This ensures the header is compilable > >>>>> by itself, and has no implicit inclusion criteria for use. This is > >>>>> sometimes, but not always true in KiCad. (In this case, there is no > >>>>> .cpp anyway, so the unit test was the first CPP to attempt to include > >>>>> by itself). > >>>>> > >>>>>> The problem is all of the inline functions defined in convert_to_biu.h. > >>>>>> Until they are replaced with per application equivalents, we will > >>>>>> continue to have to compile every common source file multiple times > >>>>>> that > >>>>>> use anything in this file. > >>>>> > >>>>> Right, but in the case of dialog_page_settings.cpp in particular, the > >>>>> only definition actually needed from here was IU_PER_MILS, > >>>>> which can be equivalently retrieved, via the virtual BASE_SCREEN > >>>>> interface, from the derived class, where IU_PER_MILS is properly set > >>>>> for each compiled target (eeschema, pl_editor, pcbnew). > >>>>> > >>>>>> This is odd as they are both in the COMMON_SRCS list. Actually, > >>>>>> color.cpp is included twice. Once in KICAD_SRCS and once in > >>>>>> COMMON_SRCS > >>>>>> which includes KICAD_SRCS. We should remove color.cpp from either > >>>>>> KICAD_SRCS or COMMON_SRCS. I see no need to build it twice. > >>>>> > >>>>> Even more odd. Removing one of the colors.cpp from the > >>>>> common/CMakeLists.cpp didn't stop qa_common having to specifiy it > >>>>> again, though. > >>>>> > >>>>> For colors.cpp specifically, this is an issue which will be resolved > >>>>> when, as promised in colors.h, the removal of legacy support will make > >>>>> the global colour table obsolete. So it's perhaps more of a curiosity > >>>>> than an issue? > >>>>> > >>>>> Regardless, if anyone can shed light on this, I'd appreciate it, as it > >>>>> probably indicates something fishy somewhere? > >>>>> > >>>>> Cheers, > >>>>> > >>>>> John > >>>>> > >>>>> [1]: > >>>>> http://docs.kicad-pcb.org/doxygen/md_Documentation_development_coding-style-policy.html#header_depends > >>>>> _______________________________________________ Mailing list: https://launchpad.net/~kicad-developers Post to : kicad-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~kicad-developers More help : https://help.launchpad.net/ListHelp