Fedora Merge Review: libXcursor http://cvs.fedora.redhat.com/viewcvs/devel/libXcursor/ Initial Owner: sandmann
Review: + koji build =>http://koji.fedoraproject.org/koji/buildinfo?buildID=135668 + verified upstream source as 096d0e538d37fd865705e5f45b0e96c7294c1f2f libXcursor-1.1.10.tar.bz2 + rpmlint output can be ignored libXcursor.i686: E: file-in-usr-marked-as-conffile /usr/share/icons/default/index.theme + Package libXcursor-1.1.10-2.fc13.i686 Provides: config(libXcursor) = 1.1.10-2.fc13 libXcursor.so.1 Requires: libX11.so.6 libXcursor.so.1 libXfixes.so.3 libXrender.so.1 libc.so.6 libc.so.6(GLIBC_2.0) libc.so.6(GLIBC_2.1) libc.so.6(GLIBC_2.1.3) libc.so.6(GLIBC_2.3) libc.so.6(GLIBC_2.3.4) libc.so.6(GLIBC_2.4) rtld(GNU_HASH) + Package libXcursor-devel-1.1.10-2.fc13.i686 Provides: pkgconfig(xcursor) = 1.1.10 Requires: /usr/bin/pkg-config libXcursor.so.1 pkgconfig(x11) pkgconfig(xfixes) pkgconfig(xproto) pkgconfig(xrender) APPROVED.
Sorry, but you missed a couple of issues: - md5sum is 7dcdad1c10daea872cb3355af414b2ca, both for upstream as well as for the file from lookaside cache - timestamps not preserved during iconv - timestamp of index.theme not preserved during install - permissions are not - /usr/share/ hardcoded, although commented out - devel package missing "Requires: xorg-x11-proto-devel" - why do we still inherit Bluecurce in index.theme? - make is not verbose, so RPM_OPT_FLAGS cannot be verified CC cursor.lo CC display.lo CC file.lo CC library.lo CC xlib.lo CCLD libXcursor.la I know you are doing a lot of reviews, but IMO the quality must not suffer from the quantity.
(In reply to comment #2) > - permissions are not Ignore that one.
(In reply to comment #2) > Sorry, but you missed a couple of issues: > > - md5sum is 7dcdad1c10daea872cb3355af414b2ca, both for upstream as well as for > the file from lookaside cache Thanks. I started using sha1sum > - timestamps not preserved during iconv I wonder how can we preserve timestamps when using iconv. Can you please provide me an example? > - timestamp of index.theme not preserved during install Thanks will update in SPEC. > - permissions are not > - /usr/share/ hardcoded, although commented out will better remove comment then. > - devel package missing "Requires: xorg-x11-proto-devel" Ok > - why do we still inherit Bluecurce in index.theme? Need to check with maintainers..... > - make is not verbose, so RPM_OPT_FLAGS cannot be verified > CC cursor.lo > CC display.lo > CC file.lo > CC library.lo > CC xlib.lo > CCLD libXcursor.la will take care of this. > > I know you are doing a lot of reviews, but IMO the quality must not suffer from > the quantity. Thanks. I know that but reason of doing these merge reviews in hurry is I was long time waiting for some time to give to review these but finally I decided to complete this as maintainers himself not interested for such minor thing changes in spec. Feel free to cross verify my reviews :) I really don't want to start again debate on why merge-reviews maintainers are not taking responsibility to change spec according to packaging guidelines.
(In reply to comment #4) > (In reply to comment #2) > > - md5sum is 7dcdad1c10daea872cb3355af414b2ca, both for upstream as well as for > > the file from lookaside cache > Thanks. I started using sha1sum Yeah, after the third mismatch I got it. The review Guidelines still state "The sources used to build the package must match the upstream source, as provided in the spec URL. Reviewers should use md5sum for this task." If you use something different, please make notice in the bug. > > - timestamps not preserved during iconv > I wonder how can we preserve timestamps when using iconv. Can you please > provide me an example? Second example on https://fedoraproject.org/wiki/PackageMaintainers/PackagingTricks#Convert_encoding_to_UTF-8 > Thanks. I know that but reason of doing these merge reviews in hurry is I was > long time waiting for some time to give to review these but finally I decided > to complete this as maintainers himself not interested for such minor thing > changes in spec. Sad but true, I know this very well from other merge reviews. Keep on your good work!
updated package in F-13