kicad-developers team mailing list archive

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

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