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

[Mark Wielaard]

Hi Andrew,

Thanks for the constructive email. We were both a bit too over eager to
get "our" builds to be fixed first/best. Thanks also for talking a
little off list to make sure we were keeping constructive going forward.

On Thu, 2011-02-17 at 21:02 +0000, Dr Andrew John Hughes wrote:
> On 19:36 Thu 17 Feb     , Mark Wielaard wrote:
> > 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.

That does take a lot of extra time though. I really do think we
shouldn't force people to do multiple full builds, except for the
default one and the variant that they are hacking/testing. It would be
nice if we could somehow integrate this like we do with other
alternative VMs with --enable-additional-vms. So you can easily build
them during the main build, without having to rebuild everything else.
And people might find it nice to try them both in their builds.

Also, could we have an alias for "hotspot=next" or something? Assuming
that we only support "current" and "next" hotspot, then you don't have
to remember which particular version it represents (also easier for
making sure a buildslave tracks them).

Xerxes offered to change one of his buildslaves to track hs20, so
hopefully that will also help making sure that setup doesn't
accidentally break.

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

Agreed. And sorry for not formally announcing the existence of the
buildbot. Xerxes and I were still experimenting with various setups. And
I admit I was a little ashamed of my hacky master.cfg setup. But I
checked them all in and will send a more formal announcement with some
instructions how anybody can add a buildslave or suggest changes in the
current setup.

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

And also my apologies. I wasn't exactly nice in formulating my own
attempts at "helping" and asking for help. We are all a little short on
time.

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

We probably should discuss that a bit more. It was unfair of me to treat
these as if the were "normal" patches. You take these security issues
on, and really do make sure they get applied as soon as possible, when
the underlying issues are announced to the world at large. But we might
need a bit more formal "security team" approach to make sure you don't
get overwhelmed by them. Should we start a new thread on how to get more
help with this process? I admit to not exactly know how you get into
possession of these embargoed security fixes ahead of time, who
embargoes them, what the process is if the happen to become public
before the embargo date, or who else is involved, etc.

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

You do take on a lot of the grunt work on making sure IcedTea keeps up
to date and working nicely. Thanks. I admit that I was feeling like the
only person caring about making sure the "alternative" runtimes kept
working. Lets see if we can make this a bit smoother for all involved,
whatever runtime/setup they care about. That is really why Xerxes and I
started to work on the buildbot setup. See upcoming email.

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

Yes, felt it was discussed at length through the bug report and that it
was "urgent" to fix asap, seeing that the autobuilders were all broken
(because the only ones having been setup do always include all
alternative runtimes). I'll add a more basic/plain build config to one
of the buildslaves soon. And will try to have a bit more patience.

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

Grin. Well, to be honest, that was how I was feeling. Lets agree to be
more aggressive/speedy in reviewing other people patches.

> I hope that clears the air a bit.

Yes, it did. Appreciated. Hope my reply do the same.

Thanks,

Mark