[Insight-developers] GDCM empty cxx files

Bill Lorensen bill.lorensen at gmail.com
Sun Mar 27 10:13:02 EDT 2011 

Ok, I'll abandon my patch. I'm well aware of the effort to get the last
warnings and tests fixed. I would concentrate on the warning on the GDCM
dashboard that appear in ITK. And please don't wait until they are all fixed
before submitting them to gerrit. I'll help shepard them through gerrit.

Thanks,

Bill

On Sun, Mar 27, 2011 at 6:26 AM, Mark Roden <mmroden at gmail.com> wrote:

> Yes.  The new stuff is packed with warnings and failing tests.  It's
> slow going, but it's isolated from ITK's development and contains its
> own test suite.
>
> The dashboard can already indicate warnings that are 'new' (with a
> little +2 or whatever), those can be clicked on, and the warning
> found.  But I'm guessing that that's not enough.
>
> One feature that would be very helpful, especially in isolating the
> warnings from the ITK dashboard and could be done in parallel to these
> code cleanup efforts, is to have warnings filterable by keywords.
> That is, OK, I have x number of warnings, but look for just the ones
> containing 'gdcm', 'vnl', 'spatial', etc, or remove warnings
> containing known keywords.  That way, I can see just the warnings
> relevant to me (gdcm), and you can see warnings that are relevant to
> you (not gdcm).
>
> This solution absolutely doesn't solve the problem at hand, but it
> could at least allow you to see new warnings and errors as they crop
> up.  I'm the person doing this warning reduction, and it's slow going.
>  The 'easy' warnings were cleaned up many months ago, and the ones
> that are left are very tricky indeed, like 4333 and 4355.  If I solve
> them in ITK, because the code is so different from GDCM main, we'll be
> back to square one once the new GDCM gets added in for the networking
> and streaming functionality.  I'd rather not have to do this again in
> a few months, but just get it done now.
>
> So the plan is to fix warnings in GDCM main, port back to ITK main
> once finished. Last week, I cleaned up three tests that had been
> failing for quite a while, and fixing a test can be an all-day (or
> longer) affair.  I'm currently working on the streaming writing tests
> and the networking tests.  Tests like TestVRDS and other low-level
> tests have functionality with which I'm not familiar, and so it's hard
> for me to predict completion times.
>
> Mark
>
>
> On Sat, Mar 26, 2011 at 6:01 PM, Bill Lorensen <bill.lorensen at gmail.com>
> wrote:
> > As I said before, I don't care how it happens, but we must get the
> warnings
> > and failing tests to zero for gdcm. The current situation makes it
> difficult
> > to detect new warnings that are introduced.
> >
> > Looking at the gdcm dashboard, I see a toolkit that has many warnings and
> > failing tests:
> > http://public.kitware.com/dashboard.php?name=gdcm
> >
> > The Release 2.0 still has warnings and is only built on 2 platforms.
> >
> > The situation on the itk dashboard has hardly improved since the start of
> > itk4.
> >
> > What is the plan?
> >
> > Bill
> >
> >
> > On Sat, Mar 26, 2011 at 5:57 PM, Mark Roden <mmroden at gmail.com> wrote:
> >>
> >> Hi Alex,
> >>
> >> The reality is, the tests just aren't there in ITK proper.  Fixing the
> >> warnings in ITK as they stand now will cause tests in GDCM proper to
> >> fail, which means that many files can't be read.  Bill's most recent
> >> submission, for instance, will cause two GDCM tests to fail, but won't
> >> cause any ITK tests to fail, since the test coverage just isn't there.
> >>  Couple that with the lack of response I got for my submissions in
> >> ITK, and it's just faster for me to work in GDCM proper, fix the
> >> warnings there with as much test coverage as I can get (and getting
> >> those tests solved is no mean feat, especially since they're exposing
> >> some interesting corner cases), and then port to ITK knowing that the
> >> tests are working.
> >>
> >> The breaking test in ITK, the one that's not been working for many
> >> months now, is just not a correct test.  The test itself was made a
> >> test using a broken, invalid DICOM test, and then breaks because GDCM
> >> refuses to read the broken image, which is the proper behavior as
> >> specified in the DICOM specification.  The test still hasn't been
> >> fixed after both Mathieu and I have repeatedly pointed out that it's a
> >> broken test, and attempts to fix it have been ignored.
> >>
> >> So, I believe that working in GDCM proper is the right thing to do to
> >> fix these warnings in some kind of timely manner.  The ITK development
> >> process has just been too slow, and I want to ensure that
> >> functionality isn't broken in ITK that works in GDCM proper due to
> >> attempts to fix warnings quickly.  I would also love it if the GDCM
> >> tests and testing library could be included in ITK's test suite, but I
> >> don't know how that would work.
> >>
> >> Mark
> >>
> >> On Sat, Mar 26, 2011 at 12:41 AM, Alexandre GOUAILLARD
> >> <agouaillard at gmail.com> wrote:
> >> > mark,
> >> >
> >> > as indicated many times,
> >> > our duty goes to ITK and not to gdcm proper.
> >> > The priority is to fix the warning on the itk dashboard first.
> >> > Brad king mentionned you could modify in ITK and backport to gdcm
> later.
> >> > I explained to you how you could also have a separate git rep. to be
> >> > the buffer between gdcm and itk if need be.
> >> >
> >> > now, our duty goes to ITK. The priority is to make the ITK dashboard
> >> > green
> >> >
> >> > alex.
> >> >
> >> >
> >> > On Sat, Mar 26, 2011 at 6:14 AM, Bill Lorensen <
> bill.lorensen at gmail.com>
> >> > wrote:
> >> >> Whatever is the best approach to fix the gdcm warnings. They are the
> >> >> major
> >> >> source of warnings are really getting annoying. They have been
> present
> >> >> since
> >> >> last summer.  I will continue to fix gdcm warnings until I see them
> >> >> dropping
> >> >> through another means.
> >> >>
> >> >> On Fri, Mar 25, 2011 at 6:10 PM, Mark Roden <mmroden at gmail.com>
> wrote:
> >> >>>
> >> >>> Ah, thanks for that.
> >> >>>
> >> >>> Since it took so long for my last gerrit patch to get a review, I've
> >> >>> switched to fixing tests and warnings in the gdcm main branch.  My
> >> >>> hope was to be able to drop that chunk of code into ITK, with the
> >> >>> warnings fixed there.  Many of them have already been fixed in ITK,
> >> >>> but moving over fixes from ITK to gdcm main produced some failing
> gdcm
> >> >>> tests.  These tests don't exist in ITK (hence removing the GDCM
> build
> >> >>> option from the ITK cmake options), and the ITK gdcm tests appear to
> >> >>> be building and running just fine (with the exception of IO test 5,
> >> >>> and that looks to be the same broken test it's always been, and
> should
> >> >>> be removed from the test suite).
> >> >>>
> >> >>> Is this approach a valid one?  I mean, it's taken around 2 weeks for
> >> >>> someone to review an ITK gerrit patch from me, but in the meantime,
> >> >>> I've been able to make much faster progress in the gdcm main branch
> >> >>> with this approach.  And, I think, a safer one, since there is more
> >> >>> complete test coverage.
> >> >>>
> >> >>> Mark
> >> >>>
> >> >>> On Fri, Mar 25, 2011 at 3:05 PM, Bill Lorensen
> >> >>> <bill.lorensen at gmail.com>
> >> >>> wrote:
> >> >>> > I have the fix in my gerrit patch.
> >> >>> >
> >> >>> > On Fri, Mar 25, 2011 at 3:33 PM, Mark Roden <mmroden at gmail.com>
> >> >>> > wrote:
> >> >>> >>
> >> >>> >> I thought I submitted a patch for this as well, but it probably
> >> >>> >> didn't
> >> >>> >> make it over the ITK modularization boundary.
> >> >>> >>
> >> >>> >> The last patch I submitted was done after the ITK changeover, but
> I
> >> >>> >> guess the changeover was ongoing when I submitted the patch, and
> so
> >> >>> >> now I have to redo it.  That's the
> >> >>> >> getting-rid-of-superfluous-cmake-options patch.
> >> >>> >>
> >> >>> >> On Fri, Mar 25, 2011 at 12:21 PM, Ryan, William J.
> >> >>> >> <ryan.william at mayo.edu> wrote:
> >> >>> >> > I think Mark Roden had a patch submitted that may fix this, but
> >> >>> >> > I'm
> >> >>> >> > not
> >> >>> >> > 100%
> >> >>> >> > sure about that.  Let's see what he has to say.
> >> >>> >> >
> >> >>> >> >
> >> >>> >> >
> >> >>> >> >
> >> >>> >> >
> >> >>> >> >   Bill Ryan
> >> >>> >> >
> >> >>> >> >
> >> >>> >> >
> >> >>> >> > From: insight-developers-bounces at itk.org
> >> >>> >> > [mailto:insight-developers-bounces at itk.org] On Behalf Of Bill
> >> >>> >> > Lorensen
> >> >>> >> > Sent: Friday, March 25, 2011 1:23 PM
> >> >>> >> > To: Mathieu Malaterre
> >> >>> >> > Cc: Insight Developers
> >> >>> >> > Subject: Re: [Insight-developers] GDCM empty cxx files
> >> >>> >> >
> >> >>> >> >
> >> >>> >> >
> >> >>> >> > Mathieu,
> >> >>> >> >
> >> >>> >> > We still support VS7.1. I would like to submit a patch that
> >> >>> >> > removes
> >> >>> >> > these
> >> >>> >> > files from the lib and see if it works.
> >> >>> >> >
> >> >>> >> > Unfortunately, the VS generator does not work with the
> >> >>> >> > modularized
> >> >>> >> > ITK.
> >> >>> >> > We
> >> >>> >> > may not be able to thoroughly test it until the patched cmake
> is
> >> >>> >> > released.
> >> >>> >> >
> >> >>> >> > Thanks,
> >> >>> >> >
> >> >>> >> > Bill
> >> >>> >> >
> >> >>> >> > On Fri, Mar 25, 2011 at 2:13 PM, Mathieu Malaterre
> >> >>> >> > <mathieu.malaterre at gmail.com> wrote:
> >> >>> >> >
> >> >>> >> > From the top of my head, this was V7.1 with /MT as compilation
> >> >>> >> > flag
> >> >>> >> > (instead of the default /MD used by Cmake). Can't remember if
> >> >>> >> > this
> >> >>> >> > was
> >> >>> >> > using shared or static lib.
> >> >>> >> >
> >> >>> >> > Does anyone still have this compiler ?
> >> >>> >> >
> >> >>> >> > HTH
> >> >>> >> >
> >> >>> >> > On Fri, Mar 25, 2011 at 7:06 PM, Bill Lorensen
> >> >>> >> > <bill.lorensen at gmail.com>
> >> >>> >> > wrote:
> >> >>> >> >> Mathieu,
> >> >>> >> >>
> >> >>> >> >> Several files including gdcmObject.cxx have no code in them.
> >> >>> >> >> Your
> >> >>> >> >> comment
> >> >>> >> >> in
> >> >>> >> >> gdcmObject.cxx says:
> >> >>> >> >>   // Don't ask why, but this is EXTREMELY important on Win32
> >> >>> >> >>   // Apparently the compiler is doing something special the
> >> >>> >> >> first
> >> >>> >> >> time
> >> >>> >> >> it
> >> >>> >> >> compiles
> >> >>> >> >>   // this instanciation unit
> >> >>> >> >>   // If this fake file is not present I get an unresolved
> symbol
> >> >>> >> >> for
> >> >>> >> >> each
> >> >>> >> >> function
> >> >>> >> >>   // of the gdcm::Object class
> >> >>> >> >>
> >> >>> >> >> When linking these "empty" files, VS10 reports several
> warnings:
> >> >>> >> >>  warning LNK4221: This object file does not define any
> >> >>> >> >> previously
> >> >>> >> >> undefined
> >> >>> >> >> public symbols, so it will not be used by any link operation
> >> >>> >> >> that
> >> >>> >> >> consumes
> >> >>> >> >> this library
> >> >>> >> >>
> >> >>> >> >> I have removed this file from the lib as well as
> >> >>> >> >> gdcmProgressEvent.cxx, gdcmString.cxx, gdcmException.cxx
> >> >>> >> >> gdcmDeflateStream.cxx and gdcmByteSwap.cxx
> >> >>> >> >>
> >> >>> >> >> When I build the modified lib, I do not get the warnoings, nor
> >> >>> >> >> do I
> >> >>> >> >> get
> >> >>> >> >> any
> >> >>> >> >> errors.
> >> >>> >> >>
> >> >>> >> >> What compiler was giving the errors you mention in the
> comments?
> >> >>> >> >>
> >> >>> >> >> Bill
> >> >>> >> >>
> >> >>> >> >
> >> >>> >> >
> >> >>> >> > --
> >> >>> >> > Mathieu
> >> >>> >> >
> >> >>> >> >
> >> >>> >
> >> >>> >
> >> >>
> >> >>
> >> >> _______________________________________________
> >> >> Powered by www.kitware.com
> >> >>
> >> >> Visit other Kitware open-source projects at
> >> >> http://www.kitware.com/opensource/opensource.html
> >> >>
> >> >> Kitware offers ITK Training Courses, for more information visit:
> >> >> http://kitware.com/products/protraining.html
> >> >>
> >> >> Please keep messages on-topic and check the ITK FAQ at:
> >> >> http://www.itk.org/Wiki/ITK_FAQ
> >> >>
> >> >> Follow this link to subscribe/unsubscribe:
> >> >> http://www.itk.org/mailman/listinfo/insight-developers
> >> >>
> >> >>
> >> >
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.itk.org/mailman/private/insight-developers/attachments/20110327/b8845aed/attachment-0001.htm>

More information about the Insight-developers mailing list