GNOME Bugzilla – Bug 623473
zlib should be checked with pkg-config
Last modified: 2010-09-29 15:16:34 UTC
Created attachment 165177 [details] [review] Suggested patch It might not be installed in /usr. Makes cross-compilation easier.
+PKG_CHECK_EXISTS([zlib], [found_zlib=yes], [found_zlib=no]) +if test "x$found_zlib" = "xyes" ; then + PKG_CHECK_MODULES([ZLIB], [zlib]) +else No need for this double check with EXISTS + MODULES; you can simply do PKG_CHECK_MODULES([ZLIB],[zlib],[found_zlib=yes],[found_zlib=no]) if test "x$found_zlib" = "xno"; then ...
(In reply to comment #1) > No need for this double check with EXISTS + MODULES; you can simply do Yes, that's what I was using at first as that's what I do in other packages, however I noticed that way the failing check appears twice on the log, so I decided to give this version a try.
For the record, it's not nice to remove the author from the patch. I found the problem, I reported it, and I provided the fix. The modifications done on my patch were trivial and don't deserve the author to be changed. I could have done them in a couple minutes myself. Thanks for making 'git log --author=felipe.contreras' return nothing. Way to incentivize contributions.
I fixed this based on comment 1, which specifically proposed to do something different from your patch. If I directly commit a provided patch, I normally use --author.
(In reply to comment #4) > I fixed this based on comment 1, which specifically proposed to do something > different from your patch. My patch is checking zlib with pkg-config. The final patch does exactly the same thing. This is the interdiff: -PKG_CHECK_MODULES([ZLIB], [zlib], [found_zlib=yes], [found_zlib=no]) -if test "x$found_zlib" = "xno" ; then +PKG_CHECK_EXISTS([zlib], [found_zlib=yes], [found_zlib=no]) +if test "x$found_zlib" = "xyes" ; then + PKG_CHECK_MODULES([ZLIB], [zlib]) +else Such a small diff makes you instead of me author of the patch? Besides, as I said, I *started* which such version, but I thought mine one was better, but could have easily switched back. > If I directly commit a provided patch, I normally use --author. Why? git has a field for author, and a field for committer. I do what good maintainers do; I ask the contributor of the patch to make the modifications by himself, and then I commit the patch as-is. This way the contributor learns the ropes of the project, I, the maintainer, do minimal effort, and the contributor gets to have his name on the log. Even if I do minimal changes on the patch I leave the contributor as "author" and I add in the commit message "Modifications by Felipe Contreras", which closely resembles what happened. I forgot another thing I did with the patch: I *tested* it. Did you? I see you forgot to make the changes I did gio/Makefile.am, so probably your patch doesn't work, which is understandable, because you are not the author.
Can we stop this now ? What you are doing here is not 'incentivizing' me to take any more patches from you either...
Stop what? Stop trying to improve things? As I said: I found the problem, I reported it, I provided the fix, and I tested the fix. This took a considerable part of my time, specially the testing. And you are the author? Excuse me, but I feel a little bit taken advantage from. If you don't care what I, and other contributors, might feel when you take the authorship from them, then I say you are not open to constructive feedback. I think you should: 1) Reconsider taking authorship from patches you are not the author, and instead add a comment saying that you made some changes 2) Give your contributors a chance to improve the patches by themselves 3) Be humble about your patches; the fact that you are the maintainer doesn't mean that all your patches are gold, instead, post the patches the same way your contributors did, on bugzilla, and wait for comments, just like your contributors do 4) Listen from comments from your contributors; open source is all about collaboration Other successful projects, like git and linux, do precisely this. But more importantly: 5) Accept that you screwed up this patch by ignoring gio/Makefile.am If you don't do this, specially 5), and instead make things personal, I think the only one who looses is the project.
(In reply to comment #7) > Stop what? Stop trying to improve things? You are perfectly attributed by name, "based on" is jargon for "that guy wrote it and I changed it". You wrongly make it sound as if Matthias intended to harm you and steal your ideas. Please don't make a fuss about standard practise.
Its a 3 line configure patch, for crying out loud. I'm sorry if my git usage is not up to your standards. This is my last comment on this matter, I'm off to vacation now.
(In reply to comment #8) > (In reply to comment #7) > > Stop what? Stop trying to improve things? > > You are perfectly attributed by name, "based on" is jargon for "that guy wrote > it and I changed it". No, "based on" could mean a number of things, including "I wrote it all from scratch, but I based it on X". But this is not about semantics; there's field called "author" and I'm the author. Period. > You wrongly make it sound as if Matthias intended to harm > you and steal your ideas. No, I'm not saying he *intended* that, but you can do harm without intention. > Please don't make a fuss about standard practise. I'm not making a fuss. I would expect a good maintainer to say: "You are right, I'll try to keep the authorship from now own. How about you send another patch fixing the remaining issues? I'll keep you as author this time." Is that not sensible?
(In reply to comment #9) > Its a 3 line configure patch, for crying out loud. It's not: 2 files changed, 12 insertions(+), 6 deletions(-) And in my book, small patches are actually a good thing. In linux is not uncommon for one line patches to have a commit message a page long. It's not size that matters. > I'm sorry if my git usage is not up to your standards. That's easy to fix, just do your best to keep the author. > This is my last comment on this matter, I'm off to vacation now. Ok, the purpose of this patch was to allow zlib to be on locations other than /usr, my patch fixes that, but your patch doesn't. You don't care about that? Then what's the point of merging your patch? Anyway, I just found that zlib is also missing from the gio pc file. I would gladly: 1) Test that the current functionality is indeed broken (most probably it is since on the road of creating this patch I tested a similar version to your patch and it didn't work) 2) File a new bug report 3) Attach and test a new patch that fixes the issues 4) Make sure that the patch also addresses the missing zlib dependency on gio's pkgconfig Keep in mind that in order to properly test this, I cross-compile both zlib and glib (in order to avoid the system's zlib), and then I have to build a gio app. So it's not a matter of "just writing a few lines". But if there's a minor comment on the patch, and Matthias does the minor change, and then takes authorship of the patch, I don't really feel motivated to do all that.
Indeed, I was right: gzlibcompressor.c:26: fatal error: zlib.h: No such file or directory Hopefully now the code won't be dropped, or "rewritten" without actually testing it as I did: bug #623473.