[Insight-developers] GDCM empty cxx files

Mark Roden mmroden at gmail.com
Mon Mar 28 09:56:48 EDT 2011 

Qualified people.  As you can see from the reviews Bill listed, at
least four reviewers per.

On Mon, Mar 28, 2011 at 4:28 AM, David Cole <david.cole at kitware.com> wrote:
> Are you requesting reviews explicitly from qualified individuals, or are you
> expecting people to find your changes in gerrit and volunteer to be
> reviewers?
>
> On Sun, Mar 27, 2011 at 1:04 PM, Mark Roden <mmroden at gmail.com> wrote:
>>
>> Hi Hans,
>>
>> If it's acceptable to wallpaper over the warnings, we can go ahead and
>> pragma them out for now.
>>
>> As for the lack of response, my understanding is that I have to go
>> through the gerrit process to submit fixes.  I went through said
>> process, and my submission received no attention.  This happened
>> twice.  So, if my understanding of the process is wrong and I can
>> expedite these things (should I be sending poking emails to people to
>> check my commits?), then please correct me.
>>
>> Mark
>>
>> On Sun, Mar 27, 2011 at 7:36 AM, Johnson, Hans J <hans-johnson at uiowa.edu>
>> wrote:
>> > Mark,
>> >
>> > If the larger GDCM issues are being addressed outside of the ITK
>> > development process, then perhaps you could just silence the current ITK
>> > warnings and failing tests, and explicitly state in the commit message
>> > that this is a short term "problem avoidance in ITK" because the
>> > "problem
>> > solution is being addressed out-of-band and will contributed back to ITK
>> > proper when it is complete".   I may not be correct here, but the
>> > biggest
>> > problem with the GDCM compiler warnings and testing problems is that it
>> > is
>> > adding a lot of noise to the efforts of squashing other problems.
>> >
>> > Regarding you comments of ITK developers not responding to your request
>> > to
>> > fist test.  I want to remind you the YOU are THEY,  that is to say that
>> > you are empowered (and likely the most qualified) to fix these problems.
>> >
>> > Hans
>> >
>> > On 3/26/11 4: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
>> >>>>
>> >>>>
>> >>>
>> >>_______________________________________________
>> >>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
>> >
>> >
>> >
>> > ________________________________
>> > Notice: This UI Health Care e-mail (including attachments) is covered by
>> > the Electronic Communications Privacy Act, 18 U.S.C. 2510-2521, is
>> > confidential and may be legally privileged.  If you are not the intended
>> > recipient, you are hereby notified that any retention, dissemination,
>> > distribution, or copying of this communication is strictly prohibited.
>> >  Please reply to the sender that you have received the message in error,
>> > then delete it.  Thank you.
>> > ________________________________
>> >
>> _______________________________________________
>> 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
>
>

More information about the Insight-developers mailing list