FYI: Fix PR632: patches/security/20110215/6878713.patch breaks shark zero build

[Dr Andrew John Hughes]

On 19:36 Thu 17 Feb     , Mark Wielaard wrote:
> Hi Andrew,
> 
> On Thu, 2011-02-17 at 17:09 +0000, Dr Andrew John Hughes wrote:
> > On 22:50 Wed 16 Feb     , Mark Wielaard wrote:
> > > On Wed, 2011-02-16 at 21:47 +0000, Dr Andrew John Hughes wrote:
> > > > On 22:28 Wed 16 Feb     , Mark Wielaard wrote:
> > > > > 2011-02-16  Xerxes Ranby  <xerxes at zafena.se>
> > > > >             Mark Wielaard  <mark at klomp.org>
> > > > > 
> > > > >     PR632: 6878713.patch breaks shark zero build
> > > > >     * patches/icedtea-stdc-limit-macros.patch
> > > > >     (openjdk/hotspot/src/share/vm/utilities/globalDefinitions_gcc.hpp):
> > > > >     Only define __STDC_LIMIT_MACROS if undefined.
> > > > >     * Makefile.am (ICEDTEA_PATCHES): Add new patch.
> > > > >     * NEWS: Updated.
> > > > > 
> > > > Did you test this with both versions of HotSpot?
> > > 
> > > No, but I don't see how it would fail, the patch is right after the
> > > patch that broke it. It is currently going through all the buildbot
> > > builders. If you have configurations you want to have regularly tested
> > > please setup a buildslave for it.
> > > 
> >
> > Please test HotSpot patches with both available HotSpot builds to avoid
> > breakage.
> 
> No. I am happy to also test the default configuration, which I did, if I
> am fixing issues on a non-default configuration  that I care about. But
> I am not going to test all random variants that might happen.
> 

I'm not asking for 'all random variants'.  That would be kind of crazy and
almost unachievable :-)

I do ask that, when there is an alternate HotSpot build available, then
patches are checked against both.  While hs20 may not be the default now,
it will become so at some point in the future (probably after the imminent
1.10 branch).  It will make things easier if it's not broken when we switch
over.  The HotSpot build option is fully documented in INSTALL.

> If you care about non-default configurations please either make sure
> that it is in the default build/test setup. Or provide a buildslave for
> it, so that it is regularly tested on all commits. That is what Xerxes
> and I did for the non-default configurations that we care about (for
> x86_64 zero/shark isn't enabled by default, and not everybody has an arm
> setup).

I don't know how to do any of this.  I didn't see any discussion or list of
what was being added to these build bots.  If I missed it, I'm sorry.  I would
have suggested that both the alternate HotSpot build (currently hs20) be checked
and also icedtea6-hg if possible.  

The alternate HotSpot build and icedtea6-hg cover what's "coming soon"
to IcedTea6 (the HotSpot build from the new stable HotSpot tree,
icedtea6-hg from the upstream OpenJDK6 tree) and allows us to find
things out early rather than getting hit by it all in one big lump
when a new tarball is released.

> 
> > I'd prefer not to have yet more work on my plate because people don't do full
> > testing of patches and commit them without any review.
> 
> It is hard to get a good feeling for tone in an email, so apologies for
> misinterpreting. But I really don't appreciate this nitpicking. 

It wasn't meant to nitpick.  It was more a plea for help.  It's great
that you want to spend time looking into this and providing a fix, but
the help is negated if it means I have to spend time doing builds to
check for breakage or start changing things.  Effectively, it's
forcing me to look into the issue immediately, so if I sound a little
negative, that's why.

> You
> committed a patch, that you didn't discuss on the list, which broke the
> zero/shark configuration that Xerxes and I care about. 

In fairness, I can't really discuss embargoed security issues :-D

No, I'll admit I didn't test Shark with the patches.  Having to do the
default and alternate HotSpot builds on three release branches and
HEAD three times over (due to us getting the security fixes in bits)
didn't leave much time for checking more estoric options.  With Shark,
in particular, I can't remember ever having had a successful build
more than a couple of times, so it's not high on my priorities.  I
understand it is with others, but then that's why I'm (hopefully)
not the only person working on IcedTea6 (though it does feel like
that sometimes).

> And that broke
> the autobuilders that we rely on to catch issues (they did send you
> email about the breakage). We quickly filed a bug report, analyzed the
> root cause, discussed some patch variants and fixed it in a way that
> minimized the impact and was as localized to the change that broke
> things as possible.
> 
> Instead of a Thank You for making sure things were fixed ASAP, we get a
> lecture on all things we should have done, but that you failed to do in
> the first place with your original commit that broke things.
> 

Well, thanks for doing this (if a little belated).  It is appreciated.
I did see the issue on IRC but I've not had time to look at it.  To be
honest, the thanks would have been immediately forthcoming if you'd
just given me or someone else chance to scan over the patch first
before committing it.  That's not really for any technical reason so
much as it turns a "oh heck what's this coming down the line"
(assuming the worst, as here) into "Oh, great you fixed that.
Thanks".

I'll admit I'm not too good at doing that myself, but that's more born
from the experience that if I do, sadly no-one looks at it until I go
and find someone and prod them...

> Cheers,
> 
> Mark
> 

I hope that clears the air a bit.

Cheers,
-- 
Andrew :)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Support Free Java!
Contribute to GNU Classpath and IcedTea
http://www.gnu.org/software/classpath
http://icedtea.classpath.org
PGP Key: F5862A37 (https://keys.indymedia.org/)
Fingerprint = EA30 D855 D50F 90CD F54D  0698 0713 C3ED F586 2A37