kicad-developers team mailing list archive

[John Beard <john.j.beard@xxxxxxxxx>]

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 <stambaughw@xxxxxxxxx> 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 <stambaughw@xxxxxxxxx> 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 <stambaughw@xxxxxxxxx> 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 <stambaughw@xxxxxxxxx> 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
> >>>>>