On Sun, May 27, 2012 at 12:54:37AM +0200, Simon Ruderich wrote: > On Tue, May 22, 2012 at 12:06:38AM +0100, Mark Brown wrote:
> > - Don't make invasive changes to upstream when you don't have to, > > otherwise you're just making work for the package maintainers. > I still don't understand why changes to 10 lines count as > invasive. They were minimal and fixed the problem (something > which wasn't possible without the patch). But I'll try to reduce > the size of the changes in future patches. The changes were not at all minimal and no changes at all to the main library were required. The changes you were doing there were all purely stylistic ones. It's those which were the big problem here. To a first approximation any change to upstream is invasive - it's something the maintainer will need to either push upstream or maintain as a diff. It's also breaking the whole one change per commit idea, making the change larger and harder to understand. This is especially true in a case like this where the actual issue and fix aren't being described, the expectation would be that these were essential changes for fixing the issue but this was hard to understand as there were customation points everywhere that was being changed which meant I had to read your patch really carefully to make sure there were no actual fixes in there that I was missing. > > I'd also add (which wasn't mentioned before) that you should actually > > describe what the problem you're reporting is directly, just providing a > > vauge description and sending a large patch doesn't do this. You just > > said that the build system was "ignoring some hardening flags" but > > didn't mention which or why. > I thought that was pretty obvious. The links I provided introduce > the concept/ideas behind the hardening flags, the reason why some > may be missing and my report mentioned that some were missing in > debian/rules and in the build system. This really wasn't at all clear - you didn't mention which flags were being ignored, nor that this wasn't actually an issue with the build system but rather (in the main library) an issue with the package not setting the right flags. I had to reverse engineer all this from your patch, which wasn't helped by the stylistic changes mixed in with it. > It's true that I didn't provide exact details which flags were > missing, but I have reported many of these bugs and duplicating > the information in the patch and the report just takes too much > time (and doesn't provide any additional insights). This is an important reason for keeping your patch clear and minimal, a clear and minimal patch can indeed be a useful description of the problem but if you do a bunch of other stuff that isn't related then this doesn't work any more. > > This all made your report much harder to work with than it needed to be, > > the overall impression was "just apply this stuff, you don't need to > > understand it". > That wasn't my intention. I provided the background links to > clarify questions about hardening. That's missing the point - general blather about hardening doesn't explain what the issue in the package is. > > Fix the actual problem. Don't introduce random stylistic changes as > > part of an "urgent" fix, and the more invasive your changes are the > > clearer you need to be about why you're making them. > That's what I tried - and the solution you've applied to zlib > does exactly the same as my original patch, it patches > contrib/minizip/Makefile (I wasn't sure that the binaries created > in Makefile.in were only example files so I patched it as well). > So I don't understand your criticism about my patch. Like I say, your patch also went and changed a large proportion of the targets in the primary zlib library which was completely unneeded and is not part of the changes in the current package! There's existing customisation points for all those targets, though I didn't bother using them as they're not shipped fixing them wouldn't need any of the changes you did there. > > I also had to reverse engineer what the problem you were trying to > > report was - your patch was not only your proposed fix, it was also your > > description of the problem. > See above. I admit it's not perfect, but the patch was simple and > I thought mentioning that the flags were missing, and where, is > enough - in addition to the patch. The main thing I spent my time doing reading the patch was going "hang on, we're adding all these new LDFLAGS things here but there's existing customiation points, what's the problem that's being fixed here?" when in actual fact there was no problem there. This is what was unclear and not simple, I was looking for something that wasn't there. Had you said that you'd decided to add a new customisation point for whatever reason that would've helped, though it does then raise the issue about why that's being done. > > isn't what you want and franly how one is supposed to guess that those > > options are appropriate is beyond me... > They are described in the hardening links I added to my original > patch. But you're right of course, hardening-check should disable > the check for them by default (and provide a way to check all > executables). I'll work on a patch when I have some time. Those links really aren't that helpful for this, there's no particular reason to trust they aren't bitrotted and the information is a bit buried (it's in the middle of pages with no ToC). They were included in the original report so there's a fair chance there's something missing there... There's also the fact that the output is just excessively verbose - it should only be reporting unexpected failures for this purpose, even if there are problems they're not going to be obvious if they're buried in with a bunch of success messages. -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
