Spec URL: http://people.redhat.com/~rdecarva/libdistorm/libdistorm.spec SRPM URL: http://people.redhat.com/~rdecarva/libdistorm/libdistorm-3.3-1.fc18.src.rpm Description: A lightweight, easy-to-use and fast disassembler/decomposer library for x86/AMD64. A decomposer means that you get a binary structure that describes an instruction rather than textual representation. Fedora Account System Username: rcvalle
A few items need to be taken care of here. I'll finish the review when the failures have been addressed. Package Review ============== Key: [x] = Pass [!] = Fail [-] = Not applicable [?] = Not evaluated [ ] = Manual review needed Issues: ======= [!]: Permissions on files are set properly. Note: See rpmlint output See: http://fedoraproject.org/wiki/Packaging/Guidelines#FilePermissions [!]: Development (unversioned) .so files in -devel subpackage, if present. Note: Unversioned so-files directly in %_libdir. See: http://fedoraproject.org/wiki/Packaging/Guidelines#DevelPackages ===== MUST items ===== C/C++: [x]: Header files in -devel subpackage, if present. [x]: Package does not contain any libtool archives (.la) [ ]: Package does not contain kernel modules. [ ]: Package contains no static executables. [x]: Rpath absent or only used for internal libs. [!]: Development (unversioned) .so files in -devel subpackage, if present. Note: Unversioned so-files directly in %_libdir. Generic: [ ]: Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines. [x]: Package successfully compiles and builds into binary rpms on at least one supported primary architecture. [ ]: %build honors applicable compiler flags or justifies otherwise. [x]: All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines. [ ]: Package contains no bundled libraries. [ ]: Changelog in prescribed format. [x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the beginning of %install. [ ]: Sources contain only permissible code or content. [x]: Each %files section contains %defattr if rpm < 4.4 [ ]: Macros in Summary, %description expandable at SRPM build time. [ ]: Package contains desktop file if it is a GUI application. [ ]: Development files must be in a -devel package [ ]: Package requires other packages for directories it uses. [ ]: Package uses nothing in %doc for runtime. [ ]: Package is not known to require ExcludeArch. [x]: Package does not contain duplicates in %files. [!]: Permissions on files are set properly. Note: See rpmlint output [x]: Fully versioned dependency in subpackages, if present. [ ]: Package complies to the Packaging Guidelines [x]: Spec file lacks Packager, Vendor, PreReq tags. [ ]: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package is included in %doc. [ ]: License field in the package spec file matches the actual license. Note: Checking patched sources after %prep for licenses. Licenses found: "GPL (v3 or later)", "Unknown or generated". 2 files have unknown license. Detailed output of licensecheck in /tmp/894338-libdistorm/licensecheck.txt [ ]: License file installed when any subpackage combination is installed. [ ]: Package consistently uses macro is (instead of hard-coded directory names). [x]: Package is named using only allowed ASCII characters. [ ]: Package is named according to the Package Naming Guidelines. [ ]: Package does not generate any conflict. Note: Package contains no Conflicts: tag(s) [x]: Package do not use a name that already exist [ ]: Package obeys FHS, except libexecdir and /usr/target. [ ]: If the package is a rename of another package, proper Obsoletes and Provides are present. [ ]: Package must own all directories that it creates. [ ]: Package does not own files or directories owned by other packages. [x]: Package installs properly. [ ]: Package is not relocatable. [ ]: Requires correct, justified where necessary. [x]: CheckResultdir [x]: Rpmlint is run on all rpms the build produces. Note: There are rpmlint messages (see attachment). [x]: Sources used to build the package match the upstream source, as provided in the spec URL. [ ]: Spec file is legible and written in American English. [x]: Spec file name must match the spec package %{name}, in the format %{name}.spec. [ ]: Package contains systemd file(s) if in need. [x]: File names are valid UTF-8. [ ]: Useful -debuginfo package or justification otherwise. [x]: Large documentation must go in a -doc subpackage. Note: Documentation size is 0 bytes in 0 files. [x]: Packages must not store files under /srv, /opt or /usr/local ===== SHOULD items ===== Generic: [x]: Reviewer should test that the package builds in mock. [x]: Buildroot is not present [x]: Package has no %clean section with rm -rf %{buildroot} (or $RPM_BUILD_ROOT) [ ]: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. [x]: Dist tag is present. [x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin. [ ]: Final provides and requires are sane (rpm -q --provides and rpm -q --requires). [ ]: Package functions as described. [ ]: Latest version is packaged. [ ]: Package does not include license text files separate from upstream. [x]: The placement of pkgconfig(.pc) files are correct. [ ]: Scriptlets must be sane, if used. [x]: SourceX tarball generation or download is documented. [!]: SourceX / PatchY prefixed with %{name}. Note: Source0 (distorm3.zip) [x]: SourceX is a working URL. [ ]: Description and summary sections in the package spec file contains translations for supported Non-English languages, if available. [ ]: Package should compile and build into binary rpms on all supported architectures. [ ]: %check is present and all tests pass. [ ]: Packages should try to preserve timestamps of original installed files. [x]: Spec use %global instead of %define. ===== EXTRA items ===== Generic: [x]: Rpmlint is run on all installed packages. Note: There are rpmlint messages (see attachment). [x]: Spec file according to URL is the same as in SRPM. [x]: Large data in /usr/share should live in a noarch subpackage if package is arched. Rpmlint ------- Checking: libdistorm-3.3-1.fc16.src.rpm libdistorm-debuginfo-3.3-1.fc16.x86_64.rpm libdistorm-3.3-1.fc16.x86_64.rpm libdistorm-devel-3.3-1.fc16.x86_64.rpm libdistorm.src: W: spelling-error Summary(en_US) diStorm -> distort, storm libdistorm.src: W: summary-not-capitalized C diStorm libdistorm.src: W: spelling-error %description -l en_US disassembler -> disassemble, disassembles, disassembled libdistorm.src: W: spelling-error %description -l en_US decomposer -> recomposed, decompose, decomposes libdistorm.src: E: no-changelogname-tag libdistorm-debuginfo.x86_64: E: no-changelogname-tag libdistorm-debuginfo.x86_64: E: debuginfo-without-sources libdistorm.x86_64: W: spelling-error Summary(en_US) diStorm -> distort, storm libdistorm.x86_64: W: summary-not-capitalized C diStorm libdistorm.x86_64: W: spelling-error %description -l en_US disassembler -> disassemble, disassembles, disassembled libdistorm.x86_64: W: spelling-error %description -l en_US decomposer -> recomposed, decompose, decomposes libdistorm.x86_64: E: no-changelogname-tag libdistorm.x86_64: W: no-soname /usr/lib64/libdistorm3.so libdistorm.x86_64: W: no-documentation libdistorm.x86_64: E: non-standard-executable-perm /usr/lib64/libdistorm3.so 0775L libdistorm-devel.x86_64: E: no-changelogname-tag libdistorm-devel.x86_64: W: no-documentation 4 packages and 0 specfiles checked; 6 errors, 11 warnings. Rpmlint (installed packages) ---------------------------- # rpmlint libdistorm-debuginfo libdistorm-devel libdistorm libdistorm-debuginfo.x86_64: I: enchant-dictionary-not-found en_US libdistorm-debuginfo.x86_64: E: no-changelogname-tag libdistorm-debuginfo.x86_64: E: debuginfo-without-sources libdistorm-devel.x86_64: E: no-changelogname-tag libdistorm-devel.x86_64: W: no-documentation libdistorm.x86_64: W: summary-not-capitalized C diStorm libdistorm.x86_64: E: no-changelogname-tag libdistorm.x86_64: W: no-soname /usr/lib64/libdistorm3.so libdistorm.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libdistorm3.so linux-vdso.so.1 libdistorm.x86_64: W: no-documentation libdistorm.x86_64: E: non-standard-executable-perm /usr/lib64/libdistorm3.so 0775L 3 packages and 0 specfiles checked; 5 errors, 5 warnings. # echo 'rpmlint-done:' Requires -------- libdistorm-debuginfo-3.3-1.fc16.x86_64.rpm (rpmlib, GLIBC filtered): libdistorm-3.3-1.fc16.x86_64.rpm (rpmlib, GLIBC filtered): /sbin/ldconfig libc.so.6()(64bit) rtld(GNU_HASH) libdistorm-devel-3.3-1.fc16.x86_64.rpm (rpmlib, GLIBC filtered): libdistorm = 3.3-1.fc16 Provides -------- libdistorm-debuginfo-3.3-1.fc16.x86_64.rpm: libdistorm-debuginfo = 3.3-1.fc16 libdistorm-debuginfo(x86-64) = 3.3-1.fc16 libdistorm-3.3-1.fc16.x86_64.rpm: libdistorm = 3.3-1.fc16 libdistorm(x86-64) = 3.3-1.fc16 libdistorm3.so()(64bit) libdistorm-devel-3.3-1.fc16.x86_64.rpm: libdistorm-devel = 3.3-1.fc16 libdistorm-devel(x86-64) = 3.3-1.fc16 Unversioned so-files -------------------- libdistorm-3.3-1.fc16.x86_64.rpm: /usr/lib64/libdistorm3.so MD5-sum check ------------- http://distorm.googlecode.com/files/distorm3.zip : CHECKSUM(SHA256) this package : d311d232e108def8acac0d4f6514e7bc070e37d7aa123ab9a9a05b9322321582 CHECKSUM(SHA256) upstream package : d311d232e108def8acac0d4f6514e7bc070e37d7aa123ab9a9a05b9322321582 Generated by fedora-review 0.3.1 (b71abc1) last change: 2012-10-16 Buildroot used: fedora-16-x86_64 Command line :/usr/bin/fedora-review -b 894338
> [!]: Development (unversioned) .so files in -devel subpackage, if present. > Note: Unversioned so-files directly in %_libdir. Did you notice that the main package would be empty then? Please always verify what fedora-review reports. The src.rpm needs a lot of work.
Actually not, because my files section was (*.so, and not *.so.*): %files %doc %{_libdir}/*.so (In reply to comment #2) > > [!]: Development (unversioned) .so files in -devel subpackage, if present. > > Note: Unversioned so-files directly in %_libdir. > > Did you notice that the main package would be empty then? > > Please always verify what fedora-review reports. > > The src.rpm needs a lot of work.
$ rpmls -p libdistorm-devel-3.3-1.fc18.x86_64.rpm -rw-r--r-- /usr/include/distorm.h -rw-r--r-- /usr/include/mnemonics.h $ rpmls -p libdistorm-3.3-1.fc18.x86_64.rpm -rwxr-xr-x /usr/lib64/libdistorm3.so qed
I don't see any empty packages in your demo. (In reply to comment #4) > $ rpmls -p libdistorm-devel-3.3-1.fc18.x86_64.rpm > -rw-r--r-- /usr/include/distorm.h > -rw-r--r-- /usr/include/mnemonics.h > > $ rpmls -p libdistorm-3.3-1.fc18.x86_64.rpm > -rwxr-xr-x /usr/lib64/libdistorm3.so > > qed
That can only be because you misunderstand Eric's review in comment 1 and my comment 2. More slowly then, okay. From comment 1, where fedora-review reported this packaging failure: [!]: Development (unversioned) .so files in -devel subpackage, if present. Note: Unversioned so-files directly in %_libdir. This is a false positive. You could not move /usr/lib64/libdistorm3.so to the -devel package, because it is the only file in the base package. And it is a run-time library, not a development file. [...] I see you've updated the spec file silently. Please bump the "Release" version when you do that, and maintain the %changelog section, too. The updated src.rpm still suffers from several issues. What you've changed silently with regard to the shared library doesn't make sense. Do run "rpmlint -i" on both the src.rpm and the built rpms. Also try a simple "rpmbuild --rebuild" with your src.rpm. It cannot be built more than once because of the weird things you do in %prep: $ rpmbuild --rebuild libdistorm-3.3-1.fc18.src.rpm Installing libdistorm-3.3-1.fc18.src.rpm warning: user rcvalle does not exist - using root warning: group rcvalle does not exist - using root warning: user rcvalle does not exist - using root warning: group rcvalle does not exist - using root Executing(%prep): /bin/sh -e /home/ms18b/tmp/rpm/tmp/rpm-tmp.lelXoK + umask 022 + cd /home/ms18b/tmp/rpm/BUILD + unzip /home/ms18b/tmp/rpm/SOURCES/libdistorm-3.3/distorm3.zip Archive: /home/ms18b/tmp/rpm/SOURCES/libdistorm-3.3/distorm3.zip replace distorm3/COPYING? [y]es, [n]o, [A]ll, [N]one, [r]ename: ^C
(In reply to comment #6) > That can only be because you misunderstand Eric's review in comment 1 and my > comment 2. > > More slowly then, okay. From comment 1, where fedora-review reported this > packaging failure: > > [!]: Development (unversioned) .so files in -devel subpackage, if present. > Note: Unversioned so-files directly in %_libdir. > > This is a false positive. You could not move /usr/lib64/libdistorm3.so to > the -devel package, because it is the only file in the base package. And it > is a run-time library, not a development file. > > [...] > > I see you've updated the spec file silently. Please bump the "Release" > version when you do that, and maintain the %changelog section, too. I didn't updated it "silently". I'm working with Eric and notified him about the update. And even before I updated it, none of the resulting packages were empty. > > The updated src.rpm still suffers from several issues. What you've changed > silently with regard to the shared library doesn't make sense. > > Do run "rpmlint -i" on both the src.rpm and the built rpms. Also try a > simple "rpmbuild --rebuild" with your src.rpm. It cannot be built more than > once because of the weird things you do in %prep: > > $ rpmbuild --rebuild libdistorm-3.3-1.fc18.src.rpm > Installing libdistorm-3.3-1.fc18.src.rpm > warning: user rcvalle does not exist - using root > warning: group rcvalle does not exist - using root > warning: user rcvalle does not exist - using root > warning: group rcvalle does not exist - using root > Executing(%prep): /bin/sh -e /home/ms18b/tmp/rpm/tmp/rpm-tmp.lelXoK > + umask 022 > + cd /home/ms18b/tmp/rpm/BUILD > + unzip /home/ms18b/tmp/rpm/SOURCES/libdistorm-3.3/distorm3.zip > Archive: /home/ms18b/tmp/rpm/SOURCES/libdistorm-3.3/distorm3.zip > replace distorm3/COPYING? [y]es, [n]o, [A]ll, [N]one, [r]ename: > ^C Just type "A". In addition, it doesn't happen for the "silently" updated Spec file anymore because I added the lines for removing the unpacked sources from previous builds, if any. Can you enumerate which "weird" things I do on prep? Btw, are you planning helping with anything?
> And even before I updated it, none of the resulting packages were empty. You still misunderstand it then. > Just type "A". Interactive builds are not acceptable. > Can you enumerate which "weird" things I do on prep? 1) not starting in a clean/empty builddir 2) not building in a %{name}-%{version} namespace dir like thousands of other packages 3) unzipping the source manually instead of using %setup for that 4) waiting for keyboard input because of 1) > Btw, are you planning helping with anything? Depends on whether you are willing to learn. At least you've started asking questions. That's good. I would use this %prep section, which solves all the problems in your one: %prep %setup -q -c %{name}-%{version} %setup -q -n %{name}-%{version}/distorm3/make/linux -D -T
(In reply to comment #8) > > And even before I updated it, none of the resulting packages were empty. > > You still misunderstand it then. Or maybe you're contradicting yourself or not being clear enough. > > > > Just type "A". > > Interactive builds are not acceptable. Quoting myself from the previous comment: "...it doesn't happen for the "silently" updated Spec file anymore because I added the lines for removing the unpacked sources from previous builds, if any." > > > > Can you enumerate which "weird" things I do on prep? > > 1) not starting in a clean/empty builddir > 2) not building in a %{name}-%{version} namespace dir like thousands of > other packages > 3) unzipping the source manually instead of using %setup for that > 4) waiting for keyboard input because of 1) For 1 and 4 see my above answer, for 2 and 3, see below. > > > > Btw, are you planning helping with anything? > > Depends on whether you are willing to learn. At least you've started asking > questions. That's good. I would use this %prep section, which solves all the > problems in your one: > > %prep > %setup -q -c %{name}-%{version} > %setup -q -n %{name}-%{version}/distorm3/make/linux -D -T It seems redundant and also unnecessarily uses the %setup macro twice. Why is it better than: rm -fr %{_builddir}/distorm3 unzip %{SOURCE0} %setup -q -n distorm3/make/linux -D -T
> Or maybe you're contradicting yourself or not being clear enough. Not at all. Eric will be able to explain it to you, because it is his review you misunderstood to begin with. > "...it doesn't happen for the "silently" updated Spec file anymore I've downloaded _two_ src.rpms from this ticket, and the second one still was suffering from the same problem. If you continue to publish updates silently in an attempt to fix issues reported to you, you need to accept that reviewers still refer to older files: $ md5sum libdistorm-3.3-1.fc18.src.rpm beac57444a21349c4a65c76f0e81cebc libdistorm-3.3-1.fc18.src.rpm Build Date: Mon 14 Jan 2013 05:26:20 PM CET That's why it's common practice to update the Release tag *and* to maintain a %changelog section in the spec file. https://fedoraproject.org/wiki/Packaging:FrequentlyMadeMistakes > Why is it better than: > > rm -fr %{_builddir}/distorm3 > unzip %{SOURCE0} > %setup -q -n distorm3/make/linux -D -T Nobody claimed anything would be "better". I only pointed out that your %prep section didn't work well and suggested a cleaner working one. Your latest one still isn't pretty, and the top builddir is still not related to %name-%version, but if it works and if you like it so much, nobody would object. ;-) What's the status of the package here now?
(In reply to comment #10) > > Or maybe you're contradicting yourself or not being clear enough. > > Not at all. Eric will be able to explain it to you, because it is his review > you misunderstood to begin with. > > > > "...it doesn't happen for the "silently" updated Spec file anymore > > I've downloaded _two_ src.rpms from this ticket, and the second one still > was suffering from the same problem. If you continue to publish updates > silently in an attempt to fix issues reported to you, you need to accept > that reviewers still refer to older files: Maybe this is because I'm working with Eric to resolve the issues reported? Until now you haven't annouced yourself as a reviewer nor as a possible sponsor, so don't expect notifications or anything made specially for you. > > $ md5sum libdistorm-3.3-1.fc18.src.rpm > beac57444a21349c4a65c76f0e81cebc libdistorm-3.3-1.fc18.src.rpm > Build Date: Mon 14 Jan 2013 05:26:20 PM CET > > That's why it's common practice to update the Release tag *and* to maintain > a %changelog section in the spec file. > https://fedoraproject.org/wiki/Packaging:FrequentlyMadeMistakes I wouldn't change it or add a changelog entry until the package is ready for the initial release. > > > > Why is it better than: > > > > rm -fr %{_builddir}/distorm3 > > unzip %{SOURCE0} > > %setup -q -n distorm3/make/linux -D -T > > Nobody claimed anything would be "better". I only pointed out that your > %prep section didn't work well and suggested a cleaner working one. Your > latest one still isn't pretty, and the top builddir is still not related to > %name-%version, but if it works and if you like it so much, nobody would > object. ;-) > > > What's the status of the package here now? If you are going to review it, the latest version was just "silently" uploaded.
Then I'll silently wait for public activity/progress in this ticket and add my comments.
Package Review ============== Key: [x] = Pass [!] = Fail [-] = Not applicable [?] = Not evaluated [ ] = Manual review needed ===== MUST items ===== C/C++: [x]: Header files in -devel subpackage, if present. [x]: ldconfig called in %post and %postun if required. [x]: Package does not contain any libtool archives (.la) [ ]: Package does not contain kernel modules. [ ]: Package contains no static executables. [x]: Rpath absent or only used for internal libs. Generic: [ ]: Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines. [x]: Package successfully compiles and builds into binary rpms on at least one supported primary architecture. [ ]: %build honors applicable compiler flags or justifies otherwise. [x]: All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines. [ ]: Package contains no bundled libraries. [ ]: Changelog in prescribed format. [x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the beginning of %install. [ ]: Sources contain only permissible code or content. [x]: Each %files section contains %defattr if rpm < 4.4 [ ]: Macros in Summary, %description expandable at SRPM build time. [ ]: Package contains desktop file if it is a GUI application. [ ]: Development files must be in a -devel package [ ]: Package requires other packages for directories it uses. [ ]: Package uses nothing in %doc for runtime. [ ]: Package is not known to require ExcludeArch. [x]: Package does not contain duplicates in %files. [x]: Permissions on files are set properly. [x]: Fully versioned dependency in subpackages, if present. [ ]: Package complies to the Packaging Guidelines [x]: Spec file lacks Packager, Vendor, PreReq tags. [ ]: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package is included in %doc. [ ]: License field in the package spec file matches the actual license. Note: Checking patched sources after %prep for licenses. Licenses found: "GPL (v3 or later)", "Unknown or generated". 2 files have unknown license. Detailed output of licensecheck in /home/rcvalle/review- libdistorm/licensecheck.txt [ ]: License file installed when any subpackage combination is installed. [ ]: Package consistently uses macro is (instead of hard-coded directory names). [x]: Package is named using only allowed ASCII characters. [ ]: Package is named according to the Package Naming Guidelines. [ ]: Package does not generate any conflict. Note: Package contains no Conflicts: tag(s) [x]: Package do not use a name that already exist [ ]: Package obeys FHS, except libexecdir and /usr/target. [ ]: If the package is a rename of another package, proper Obsoletes and Provides are present. [ ]: Package must own all directories that it creates. [ ]: Package does not own files or directories owned by other packages. [x]: Package installs properly. [ ]: Package is not relocatable. [ ]: Requires correct, justified where necessary. [x]: CheckResultdir [x]: Rpmlint is run on all rpms the build produces. Note: There are rpmlint messages (see attachment). [x]: Sources used to build the package match the upstream source, as provided in the spec URL. [ ]: Spec file is legible and written in American English. [x]: Spec file name must match the spec package %{name}, in the format %{name}.spec. [ ]: Package contains systemd file(s) if in need. [x]: File names are valid UTF-8. [ ]: Useful -debuginfo package or justification otherwise. [x]: Large documentation must go in a -doc subpackage. Note: Documentation size is 0 bytes in 0 files. [x]: Packages must not store files under /srv, /opt or /usr/local ===== SHOULD items ===== Generic: [x]: Reviewer should test that the package builds in mock. [x]: Buildroot is not present [x]: Package has no %clean section with rm -rf %{buildroot} (or $RPM_BUILD_ROOT) [ ]: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. [x]: Dist tag is present. [x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin. [ ]: Final provides and requires are sane (rpm -q --provides and rpm -q --requires). [ ]: Package functions as described. [ ]: Latest version is packaged. [ ]: Package does not include license text files separate from upstream. [ ]: Patches link to upstream bugs/comments/lists or are otherwise justified. [x]: The placement of pkgconfig(.pc) files are correct. [ ]: Scriptlets must be sane, if used. [x]: SourceX tarball generation or download is documented. [!]: SourceX / PatchY prefixed with %{name}. Note: Patch0 (distorm3.produce-debugging-information.patch) Source0 (distorm3.zip) [x]: SourceX is a working URL. [ ]: Description and summary sections in the package spec file contains translations for supported Non-English languages, if available. [ ]: Package should compile and build into binary rpms on all supported architectures. [ ]: %check is present and all tests pass. [ ]: Packages should try to preserve timestamps of original installed files. [x]: Spec use %global instead of %define. ===== EXTRA items ===== Generic: [x]: Rpmlint is run on all installed packages. Note: There are rpmlint messages (see attachment). [x]: Large data in /usr/share should live in a noarch subpackage if package is arched. Rpmlint ------- Checking: libdistorm-devel-3.3-1.fc18.x86_64.rpm libdistorm-3.3-1.fc18.x86_64.rpm libdistorm-debuginfo-3.3-1.fc18.x86_64.rpm libdistorm-3.3-1.fc18.src.rpm libdistorm-devel.x86_64: W: no-documentation libdistorm.x86_64: W: spelling-error Summary(en_US) diStorm -> distort, storm libdistorm.x86_64: W: summary-not-capitalized C diStorm libdistorm.x86_64: W: spelling-error %description -l en_US disassembler -> disassemble, disassembles, disassembled libdistorm.x86_64: W: spelling-error %description -l en_US decomposer -> recomposed, decompose, decomposes libdistorm.x86_64: W: no-soname /usr/lib64/libdistorm3.so.3.3 libdistorm.x86_64: W: no-documentation libdistorm.src: W: spelling-error Summary(en_US) diStorm -> distort, storm libdistorm.src: W: summary-not-capitalized C diStorm libdistorm.src: W: spelling-error %description -l en_US disassembler -> disassemble, disassembles, disassembled libdistorm.src: W: spelling-error %description -l en_US decomposer -> recomposed, decompose, decomposes 4 packages and 0 specfiles checked; 0 errors, 11 warnings. Rpmlint (installed packages) ---------------------------- # rpmlint libdistorm libdistorm-devel libdistorm-debuginfo libdistorm.x86_64: W: spelling-error Summary(en_US) diStorm -> distort, storm libdistorm.x86_64: W: summary-not-capitalized C diStorm libdistorm.x86_64: W: spelling-error %description -l en_US disassembler -> disassemble, disassembles, disassembled libdistorm.x86_64: W: spelling-error %description -l en_US decomposer -> recomposed, decompose, decomposes libdistorm.x86_64: W: no-soname /usr/lib64/libdistorm3.so.3.3 libdistorm.x86_64: W: no-documentation libdistorm-devel.x86_64: W: no-documentation 3 packages and 0 specfiles checked; 0 errors, 7 warnings. # echo 'rpmlint-done:' Requires -------- libdistorm-devel-3.3-1.fc18.x86_64.rpm (rpmlib, GLIBC filtered): libdistorm = 3.3-1.fc18 libdistorm-3.3-1.fc18.x86_64.rpm (rpmlib, GLIBC filtered): /sbin/ldconfig libc.so.6()(64bit) rtld(GNU_HASH) libdistorm-debuginfo-3.3-1.fc18.x86_64.rpm (rpmlib, GLIBC filtered): Provides -------- libdistorm-devel-3.3-1.fc18.x86_64.rpm: libdistorm-devel = 3.3-1.fc18 libdistorm-devel(x86-64) = 3.3-1.fc18 libdistorm-3.3-1.fc18.x86_64.rpm: libdistorm = 3.3-1.fc18 libdistorm(x86-64) = 3.3-1.fc18 libdistorm3.so.3.3()(64bit) libdistorm-debuginfo-3.3-1.fc18.x86_64.rpm: libdistorm-debuginfo = 3.3-1.fc18 libdistorm-debuginfo(x86-64) = 3.3-1.fc18 MD5-sum check ------------- http://distorm.googlecode.com/files/distorm3.zip : CHECKSUM(SHA256) this package : d311d232e108def8acac0d4f6514e7bc070e37d7aa123ab9a9a05b9322321582 CHECKSUM(SHA256) upstream package : d311d232e108def8acac0d4f6514e7bc070e37d7aa123ab9a9a05b9322321582 Generated by fedora-review 0.3.1 (b71abc1) last change: 2012-10-16 Buildroot used: fedora-18-x86_64 Command line :/usr/bin/fedora-review --rpm-spec -n rpmbuild/SRPMS/libdistorm-3.3-1.fc18.src.rpm
Spec URL: http://people.redhat.com/~rdecarva/libdistorm3/libdistorm3.spec SRPM URL: http://people.redhat.com/~rdecarva/libdistorm3/libdistorm3-3.3-1.fc18.src.rpm Description: A lightweight, easy-to-use and fast disassembler/decomposer library for x86/AMD64. A decomposer means that you get a binary structure that describes an instruction rather than textual representation. Fedora Account System Username: rcvalle
Package Review ============== Key: [x] = Pass [!] = Fail [-] = Not applicable [?] = Not evaluated [ ] = Manual review needed ===== MUST items ===== C/C++: [x]: Header files in -devel subpackage, if present. [x]: ldconfig called in %post and %postun if required. [x]: Package does not contain any libtool archives (.la) [ ]: Package does not contain kernel modules. [ ]: Package contains no static executables. [x]: Rpath absent or only used for internal libs. Generic: [ ]: Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines. [x]: Package successfully compiles and builds into binary rpms on at least one supported primary architecture. [ ]: %build honors applicable compiler flags or justifies otherwise. [x]: All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines. [ ]: Package contains no bundled libraries. [ ]: Changelog in prescribed format. [x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the beginning of %install. [ ]: Sources contain only permissible code or content. [x]: Each %files section contains %defattr if rpm < 4.4 [ ]: Macros in Summary, %description expandable at SRPM build time. [ ]: Package contains desktop file if it is a GUI application. [ ]: Development files must be in a -devel package [ ]: Package requires other packages for directories it uses. [ ]: Package uses nothing in %doc for runtime. [ ]: Package is not known to require ExcludeArch. [x]: Package does not contain duplicates in %files. [x]: Permissions on files are set properly. [x]: Fully versioned dependency in subpackages, if present. [ ]: Package complies to the Packaging Guidelines [x]: Spec file lacks Packager, Vendor, PreReq tags. [ ]: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package is included in %doc. [ ]: License field in the package spec file matches the actual license. Note: Checking patched sources after %prep for licenses. Licenses found: "GPL (v3 or later)", "Unknown or generated". 2 files have unknown license. Detailed output of licensecheck in /home/rcvalle/review- libdistorm3/licensecheck.txt [ ]: License file installed when any subpackage combination is installed. [ ]: Package consistently uses macro is (instead of hard-coded directory names). [x]: Package is named using only allowed ASCII characters. [ ]: Package is named according to the Package Naming Guidelines. [ ]: Package does not generate any conflict. Note: Package contains no Conflicts: tag(s) [x]: Package do not use a name that already exist [ ]: Package obeys FHS, except libexecdir and /usr/target. [ ]: If the package is a rename of another package, proper Obsoletes and Provides are present. [ ]: Package must own all directories that it creates. [ ]: Package does not own files or directories owned by other packages. [x]: Package installs properly. [ ]: Package is not relocatable. [ ]: Requires correct, justified where necessary. [x]: CheckResultdir [x]: Rpmlint is run on all rpms the build produces. Note: There are rpmlint messages (see attachment). [x]: Sources used to build the package match the upstream source, as provided in the spec URL. [ ]: Spec file is legible and written in American English. [x]: Spec file name must match the spec package %{name}, in the format %{name}.spec. [ ]: Package contains systemd file(s) if in need. [x]: File names are valid UTF-8. [ ]: Useful -debuginfo package or justification otherwise. [x]: Large documentation must go in a -doc subpackage. Note: Documentation size is 0 bytes in 0 files. [x]: Packages must not store files under /srv, /opt or /usr/local ===== SHOULD items ===== Generic: [x]: Reviewer should test that the package builds in mock. [x]: Buildroot is not present [x]: Package has no %clean section with rm -rf %{buildroot} (or $RPM_BUILD_ROOT) [ ]: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. [x]: Dist tag is present. [x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin. [ ]: Final provides and requires are sane (rpm -q --provides and rpm -q --requires). [ ]: Package functions as described. [ ]: Latest version is packaged. [ ]: Package does not include license text files separate from upstream. [ ]: Patches link to upstream bugs/comments/lists or are otherwise justified. [x]: The placement of pkgconfig(.pc) files are correct. [ ]: Scriptlets must be sane, if used. [x]: SourceX tarball generation or download is documented. [!]: SourceX / PatchY prefixed with %{name}. Note: Patch0 (distorm3.produce-debugging-information.patch) Source0 (distorm3.zip) [x]: SourceX is a working URL. [ ]: Description and summary sections in the package spec file contains translations for supported Non-English languages, if available. [ ]: Package should compile and build into binary rpms on all supported architectures. [ ]: %check is present and all tests pass. [ ]: Packages should try to preserve timestamps of original installed files. [x]: Spec use %global instead of %define. ===== EXTRA items ===== Generic: [x]: Rpmlint is run on all installed packages. Note: There are rpmlint messages (see attachment). [x]: Large data in /usr/share should live in a noarch subpackage if package is arched. Rpmlint ------- Checking: libdistorm3-debuginfo-3.3-1.fc18.x86_64.rpm libdistorm3-3.3-1.fc18.x86_64.rpm libdistorm3-devel-3.3-1.fc18.x86_64.rpm libdistorm3-3.3-1.fc18.src.rpm libdistorm3.x86_64: W: spelling-error Summary(en_US) diStorm -> distort, storm libdistorm3.x86_64: W: summary-not-capitalized C diStorm libdistorm3.x86_64: W: spelling-error %description -l en_US disassembler -> disassemble, disassembles, disassembled libdistorm3.x86_64: W: spelling-error %description -l en_US decomposer -> recomposed, decompose, decomposes libdistorm3.x86_64: W: no-soname /usr/lib64/libdistorm3.so.3.3 libdistorm3.x86_64: W: no-documentation libdistorm3-devel.x86_64: W: no-documentation libdistorm3.src: W: spelling-error Summary(en_US) diStorm -> distort, storm libdistorm3.src: W: summary-not-capitalized C diStorm libdistorm3.src: W: spelling-error %description -l en_US disassembler -> disassemble, disassembles, disassembled libdistorm3.src: W: spelling-error %description -l en_US decomposer -> recomposed, decompose, decomposes 4 packages and 0 specfiles checked; 0 errors, 11 warnings. Rpmlint (installed packages) ---------------------------- # rpmlint libdistorm3 libdistorm3-devel libdistorm3-debuginfo libdistorm3.x86_64: W: spelling-error Summary(en_US) diStorm -> distort, storm libdistorm3.x86_64: W: summary-not-capitalized C diStorm libdistorm3.x86_64: W: spelling-error %description -l en_US disassembler -> disassemble, disassembles, disassembled libdistorm3.x86_64: W: spelling-error %description -l en_US decomposer -> recomposed, decompose, decomposes libdistorm3.x86_64: W: no-soname /usr/lib64/libdistorm3.so.3.3 libdistorm3.x86_64: W: no-documentation libdistorm3-devel.x86_64: W: no-documentation 3 packages and 0 specfiles checked; 0 errors, 7 warnings. # echo 'rpmlint-done:' Requires -------- libdistorm3-debuginfo-3.3-1.fc18.x86_64.rpm (rpmlib, GLIBC filtered): libdistorm3-3.3-1.fc18.x86_64.rpm (rpmlib, GLIBC filtered): /sbin/ldconfig libc.so.6()(64bit) rtld(GNU_HASH) libdistorm3-devel-3.3-1.fc18.x86_64.rpm (rpmlib, GLIBC filtered): libdistorm3 = 3.3-1.fc18 Provides -------- libdistorm3-debuginfo-3.3-1.fc18.x86_64.rpm: libdistorm3-debuginfo = 3.3-1.fc18 libdistorm3-debuginfo(x86-64) = 3.3-1.fc18 libdistorm3-3.3-1.fc18.x86_64.rpm: libdistorm3 = 3.3-1.fc18 libdistorm3(x86-64) = 3.3-1.fc18 libdistorm3.so.3.3()(64bit) libdistorm3-devel-3.3-1.fc18.x86_64.rpm: libdistorm3-devel = 3.3-1.fc18 libdistorm3-devel(x86-64) = 3.3-1.fc18 MD5-sum check ------------- http://distorm.googlecode.com/files/distorm3.zip : CHECKSUM(SHA256) this package : d311d232e108def8acac0d4f6514e7bc070e37d7aa123ab9a9a05b9322321582 CHECKSUM(SHA256) upstream package : d311d232e108def8acac0d4f6514e7bc070e37d7aa123ab9a9a05b9322321582 Generated by fedora-review 0.3.1 (b71abc1) last change: 2012-10-16 Buildroot used: fedora-18-x86_64 Command line :/usr/bin/fedora-review --rpm-spec -n rpmbuild/SRPMS/libdistorm3-3.3-1.fc18.src.rpm
Filling in the many '[ ]' fields in the report would be interesting as there some issues are waiting to be discovered. https://fedoraproject.org/wiki/Packaging:ReviewGuidelines
Package Review ============== Key: [x] = Pass [!] = Fail [-] = Not applicable [?] = Not evaluated [ ] = Manual review needed ===== MUST items ===== C/C++: [x]: Header files in -devel subpackage, if present. [x]: ldconfig called in %post and %postun if required. [x]: Package does not contain any libtool archives (.la) [x]: Package does not contain kernel modules. [x]: Package contains no static executables. [x]: Rpath absent or only used for internal libs. Generic: [x]: Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines. [x]: Package successfully compiles and builds into binary rpms on at least one supported primary architecture. [x]: %build honors applicable compiler flags or justifies otherwise. [x]: All build dependencies are listed in BuildRequires, except for any that are listed in the exceptions section of Packaging Guidelines. [x]: Package contains no bundled libraries. [x]: Changelog in prescribed format. [x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the beginning of %install. [x]: Sources contain only permissible code or content. [x]: Each %files section contains %defattr if rpm < 4.4 [-]: Macros in Summary, %description expandable at SRPM build time. [-]: Package contains desktop file if it is a GUI application. [x]: Development files must be in a -devel package [-]: Package requires other packages for directories it uses. [x]: Package uses nothing in %doc for runtime. [?]: Package is not known to require ExcludeArch. [x]: Package does not contain duplicates in %files. [x]: Permissions on files are set properly. [x]: Fully versioned dependency in subpackages, if present. [x]: Package complies to the Packaging Guidelines [x]: Spec file lacks Packager, Vendor, PreReq tags. [!]: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package is included in %doc. [x]: License field in the package spec file matches the actual license. Note: Checking patched sources after %prep for licenses. Licenses found: "GPL (v3 or later)", "Unknown or generated". 2 files have unknown license. Detailed output of licensecheck in /tmp/894338-libdistorm3/licensecheck.txt [-]: License file installed when any subpackage combination is installed. [x]: Package consistently uses macro is (instead of hard-coded directory names). [x]: Package is named using only allowed ASCII characters. [x]: Package is named according to the Package Naming Guidelines. [x]: Package does not generate any conflict. Note: Package contains no Conflicts: tag(s) [x]: Package do not use a name that already exist [x]: Package obeys FHS, except libexecdir and /usr/target. [-]: If the package is a rename of another package, proper Obsoletes and Provides are present. [x]: Package must own all directories that it creates. [x]: Package does not own files or directories owned by other packages. [x]: Package installs properly. [x]: Package is not relocatable. [x]: Requires correct, justified where necessary. [x]: CheckResultdir [x]: Rpmlint is run on all rpms the build produces. Note: There are rpmlint messages (see attachment). [x]: Sources used to build the package match the upstream source, as provided in the spec URL. [x]: Spec file is legible and written in American English. [x]: Spec file name must match the spec package %{name}, in the format %{name}.spec. [?]: Package contains systemd file(s) if in need. [x]: File names are valid UTF-8. [x]: Useful -debuginfo package or justification otherwise. [x]: Large documentation must go in a -doc subpackage. Note: Documentation size is 0 bytes in 0 files. [x]: Packages must not store files under /srv, /opt or /usr/local ===== SHOULD items ===== Generic: [x]: Reviewer should test that the package builds in mock. [x]: Buildroot is not present [x]: Package has no %clean section with rm -rf %{buildroot} (or $RPM_BUILD_ROOT) [x]: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. [x]: Dist tag is present. [x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin. [x]: Final provides and requires are sane (rpm -q --provides and rpm -q --requires). [x]: Package functions as described. [x]: Latest version is packaged. [x]: Package does not include license text files separate from upstream. [x]: Patches link to upstream bugs/comments/lists or are otherwise justified. [x]: The placement of pkgconfig(.pc) files are correct. [-]: Scriptlets must be sane, if used. [x]: SourceX tarball generation or download is documented. [!]: SourceX / PatchY prefixed with %{name}. Note: Patch0 (distorm3.produce-debugging-information.patch) Source0 (distorm3.zip) [x]: SourceX is a working URL. [!]: Description and summary sections in the package spec file contains translations for supported Non-English languages, if available. [?]: Package should compile and build into binary rpms on all supported architectures. [-]: %check is present and all tests pass. [?]: Packages should try to preserve timestamps of original installed files. [x]: Spec use %global instead of %define. ===== EXTRA items ===== Generic: [x]: Rpmlint is run on all installed packages. Note: There are rpmlint messages (see attachment). [x]: Spec file according to URL is the same as in SRPM. [x]: Large data in /usr/share should live in a noarch subpackage if package is arched. Rpmlint ------- Checking: libdistorm3-3.3-1.fc18.src.rpm libdistorm3-devel-3.3-1.fc18.x86_64.rpm libdistorm3-3.3-1.fc18.x86_64.rpm libdistorm3-debuginfo-3.3-1.fc18.x86_64.rpm libdistorm3.src: W: spelling-error Summary(en_US) diStorm -> distort, storm libdistorm3.src: W: summary-not-capitalized C diStorm libdistorm3.src: W: spelling-error %description -l en_US disassembler -> disassemble, disassembles, disassembled libdistorm3.src: W: spelling-error %description -l en_US decomposer -> recomposed, decompose, decomposes libdistorm3-devel.x86_64: W: no-documentation libdistorm3.x86_64: W: spelling-error Summary(en_US) diStorm -> distort, storm libdistorm3.x86_64: W: summary-not-capitalized C diStorm libdistorm3.x86_64: W: spelling-error %description -l en_US disassembler -> disassemble, disassembles, disassembled libdistorm3.x86_64: W: spelling-error %description -l en_US decomposer -> recomposed, decompose, decomposes libdistorm3.x86_64: W: no-soname /usr/lib64/libdistorm3.so.3.3 libdistorm3.x86_64: W: no-documentation 4 packages and 0 specfiles checked; 0 errors, 11 warnings. Rpmlint (installed packages) ---------------------------- # rpmlint libdistorm3 libdistorm3-devel libdistorm3-debuginfo libdistorm3.x86_64: W: spelling-error Summary(en_US) diStorm -> distort, storm libdistorm3.x86_64: W: summary-not-capitalized C diStorm libdistorm3.x86_64: W: spelling-error %description -l en_US disassembler -> disassemble, disassembles, disassembled libdistorm3.x86_64: W: spelling-error %description -l en_US decomposer -> recomposed, decompose, decomposes libdistorm3.x86_64: W: no-soname /usr/lib64/libdistorm3.so.3.3 libdistorm3.x86_64: W: no-documentation libdistorm3-devel.x86_64: W: no-documentation 3 packages and 0 specfiles checked; 0 errors, 7 warnings. # echo 'rpmlint-done:' Requires -------- libdistorm3-devel-3.3-1.fc18.x86_64.rpm (rpmlib, GLIBC filtered): libdistorm3 = 3.3-1.fc18 libdistorm3-3.3-1.fc18.x86_64.rpm (rpmlib, GLIBC filtered): /sbin/ldconfig libc.so.6()(64bit) rtld(GNU_HASH) libdistorm3-debuginfo-3.3-1.fc18.x86_64.rpm (rpmlib, GLIBC filtered): Provides -------- libdistorm3-devel-3.3-1.fc18.x86_64.rpm: libdistorm3-devel = 3.3-1.fc18 libdistorm3-devel(x86-64) = 3.3-1.fc18 libdistorm3-3.3-1.fc18.x86_64.rpm: libdistorm3 = 3.3-1.fc18 libdistorm3(x86-64) = 3.3-1.fc18 libdistorm3.so.3.3()(64bit) libdistorm3-debuginfo-3.3-1.fc18.x86_64.rpm: libdistorm3-debuginfo = 3.3-1.fc18 libdistorm3-debuginfo(x86-64) = 3.3-1.fc18 MD5-sum check ------------- http://distorm.googlecode.com/files/distorm3.zip : CHECKSUM(SHA256) this package : d311d232e108def8acac0d4f6514e7bc070e37d7aa123ab9a9a05b9322321582 CHECKSUM(SHA256) upstream package : d311d232e108def8acac0d4f6514e7bc070e37d7aa123ab9a9a05b9322321582 Generated by fedora-review 0.3.1 (b71abc1) last change: 2012-10-16 Buildroot used: fedora-18-x86_64 Command line :/usr/bin/fedora-review -b 894338
The only problem I see is that the COPYING file is not in %docs. The COPYING file is the license text and should be included in the package.
Hopefully this comment catches all issues: https://fedoraproject.org/wiki/Packaging:ReviewGuidelines MUST: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package must be included in %doc.[4 MUST: The License field in the package spec file must match the actual license. [3] https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text A complicated %prep section makes access to local %doc files more complicated, btw. > [x]: License field in the package spec file matches the actual license. > Note: Checking patched sources after %prep for licenses. Licenses found: > "GPL (v3 or later)", "Unknown or generated". 2 files have unknown > license. Detailed output of licensecheck in > /tmp/894338-libdistorm3/licensecheck.txt > License: GPLv3 GPLv3+ actually. https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#.22or_later_version.22_licenses > [x]: %build honors applicable compiler flags or justifies otherwise. It doesn't. > [x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the > beginning of %install. Since %install automatically empties %buildroot, there is no need to "rm -fr %{buildroot}%{_libdir}" either. > [x]: Package complies to the Packaging Guidelines Not yet. > Summary: diStorm This is a very bad summary. The summary should be a short and concise description of the package. https://fedoraproject.org/wiki/Packaging/Guidelines#summary > [x]: Fully versioned dependency in subpackages, if present. https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package > [x]: Package is named according to the Package Naming Guidelines. Debatable. There is nothing that mandates adding the "lib" prefix or appending the "3". Upstream name is just "distorm". OpenSUSE have packaged it differently, just for reference: http://rpmfind.net/linux/rpm2html/search.php?query=distorm > [x]: Package does not generate any conflict. It bears a risk though to install a very generic file name, such "mnemonics.h", directly into /usr/include instead of placing files like that in a subdir. $ rpmls -p libdistorm3-devel-3.3-1.fc19.x86_64.rpm -rw-r--r-- /usr/include/distorm.h -rw-r--r-- /usr/include/mnemonics.h > [x]: Final provides and requires are sane (rpm -q --provides and rpm -q > --requires). Not yet. There still is no SONAME: libdistorm.x86_64: W: no-soname /usr/lib64/libdistorm3.so.3.3 And what the spec file tries to do about that in the %install section doesn't make sense and doesn't result in sane RPM dependencies either. > [x]: Package functions as described. Dubious. Without a libdistorm3.so it's somewhat inconvenient to compile/link with this library. Has it been tested? > [!]: Description and summary sections in the package spec file contains > translations for supported Non-English languages, if available. This '!' seems to be fedora-review's warning that the summary is not English language, but just the name of this software.
Hi Eric, I've updated both the SPEC file and packages. I think it is OK for Clint to proceed. Thanks,
Your poor attitude made me sick. Michael is a nice guy since 2003 being active in Fedora community. He is also a sponsor, you prefer too much of Eric, however you still need sponsor now. I don't have interests in such review however this package is in my to-do list.
(In reply to Christopher Meng from comment #21) > Your poor attitude made me sick. > > Michael is a nice guy since 2003 being active in Fedora community. He is > also a sponsor, you prefer too much of Eric, however you still need sponsor > now. This submission was made in conjunction with Eric purposely for him to review. Michael just showed up, and when asked about his intentions about sponsoring it, he just omitted himself. And no, I don't need a sponsor since I've lost interest. > > I don't have interests in such review however this package is in my to-do > list. Feel free to take it.
Ramon, the whole conversation is preserved in this ticket. And both the Review Process and the Sponsorship Process are documented in the Wiki. I've added review comments like many other reviewers and have been available for questions all the time.
(In reply to Michael Schwendt from comment #23) > Ramon, the whole conversation is preserved in this ticket. Yes, it is. > > And both the Review Process and the Sponsorship Process are documented in > the Wiki. I've added review comments like many other reviewers and have been > available for questions all the time. So, may I assume that you are willing to sponsor it now?
Packages aren't sponsored, people are. I have been here to review and help you. Before I could decide whether to approve this package, I would need to review its most recent update that would fix the issues that have been pointed out. That would be the minimum prerequisite for sponsorship. Many new package submitters need to contribute a few good reviews first (or submit multiple packages and show knowledge of packaging), and given your previous comments it would only be fair to expect a similar activity from you: https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group For Red Hat employees it's much easier to seek a sponsor inside Red Hat, btw.
(In reply to Michael Schwendt from comment #25) > Packages aren't sponsored, people are. I have been here to review and help > you. > > Before I could decide whether to approve this package, I would need to > review its most recent update that would fix the issues that have been > pointed out. That would be the minimum prerequisite for sponsorship. Many > new package submitters need to contribute a few good reviews first (or > submit multiple packages and show knowledge of packaging), and given your > previous comments it would only be fair to expect a similar activity from > you: The most recent update is in the same URL you downloaded the previous updates and hopefully addresses all aforementioned issues. If you find any other issues just let me know. > > https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group > > For Red Hat employees it's much easier to seek a sponsor inside Red Hat, btw.
It's probably the links in comment 14 then. Changed on March 5th, but without maintaining the %changelog. https://fedoraproject.org/wiki/Packaging:FrequentlyMadeMistakes | | Increase the "Release" tag every time you upload a new package | to avoid confusion. The reviewer and other interested parties | probably still have older versions of your SRPM lying around | to check what has changed between the old and new packages; | those get confused when the revision didn't change. The fedora-review tool evaluates the "Spec URL:" and "SRPM URL:" lines in review tickets. Running "fedora-review -b 894338" reports a few things (which I don't copy here, because it seems it's also confused, as it complains about javadoc files). "rpmlint -i libdistorm3-3.3-1.fc18.src.rpm" also finds something. The update fixes a few things. Headers are in an own dir now. Good, but it would be even better, if the %files section were explicit about that directory, so it doesn't include arbitrary stuff: %{_includedir}/distorm/ instead of %{_includedir}/* A SONAME is present now, albeit with a very strict major-minor version in addition to the major version in the libname. Has that choice been discussed anywhere? How to link with the library? Where is libdistorm3.so? $ rpmls -p libdistorm3-devel-3.3-1.fc19.x86_64.rpm drwxr-xr-x /usr/include/distorm -rw-r--r-- /usr/include/distorm/distorm.h -rw-r--r-- /usr/include/distorm/mnemonics.h Skimming over the spec file and build.log, a few mistakes remain: * rpmlint's findings (except for the false positives!) * https://fedoraproject.org/wiki/Packaging:Guidelines#All_patches_should_have_an_upstream_bug_link_or_comment * https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package * https://fedoraproject.org/wiki/Packaging:Guidelines#Compiler_flags * Why is the library built with DISTORM_STATIC and not DISTORM_DYNAMIC? * Fedora 20 will feature this: http://fedoraproject.org/wiki/Changes/UnversionedDocdirs
(In reply to Michael Schwendt from comment #27) > It's probably the links in comment 14 then. > > Changed on March 5th, but without maintaining the %changelog. > > https://fedoraproject.org/wiki/Packaging:FrequentlyMadeMistakes > | > | Increase the "Release" tag every time you upload a new package > | to avoid confusion. The reviewer and other interested parties > | probably still have older versions of your SRPM lying around > | to check what has changed between the old and new packages; > | those get confused when the revision didn't change. I don't want to add any changelog entries until I've a good initial release, which is when I'll start maintaining it. > > > The fedora-review tool evaluates the "Spec URL:" and "SRPM URL:" lines in > review tickets. > > Running "fedora-review -b 894338" reports a few things (which I don't copy > here, because it seems it's also confused, as it complains about javadoc > files). > > > "rpmlint -i libdistorm3-3.3-1.fc18.src.rpm" also finds something. Is there any action I should take? > > > The update fixes a few things. > > Headers are in an own dir now. Good, but it would be even better, if the > %files section were explicit about that directory, so it doesn't include > arbitrary stuff: > > %{_includedir}/distorm/ > instead of > %{_includedir}/* I just changed it as you pointed out. > > > A SONAME is present now, albeit with a very strict major-minor version in > addition to the major version in the libname. Has that choice been discussed > anywhere? I just changed it to "distorm" as you pointed out in previous comments as well. > > > How to link with the library? Where is libdistorm3.so? > > $ rpmls -p libdistorm3-devel-3.3-1.fc19.x86_64.rpm > drwxr-xr-x /usr/include/distorm > -rw-r--r-- /usr/include/distorm/distorm.h > -rw-r--r-- /usr/include/distorm/mnemonics.h > > > Skimming over the spec file and build.log, a few mistakes remain: > > * rpmlint's findings (except for the false positives!) > > * > https://fedoraproject.org/wiki/Packaging: > Guidelines#All_patches_should_have_an_upstream_bug_link_or_comment The patches included are for producing debugging information and setting the soname, so they don't have an upstream bug. > > * https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package > > * https://fedoraproject.org/wiki/Packaging:Guidelines#Compiler_flags > > * Why is the library built with DISTORM_STATIC and not DISTORM_DYNAMIC? This is explained at https://code.google.com/p/distorm/wiki/Build_Compilation_Environment#Helper_Macros_for_Compilation > > * Fedora 20 will feature this: > http://fedoraproject.org/wiki/Changes/UnversionedDocdirs
Spec URL: http://people.redhat.com/~rdecarva/distorm/distorm.spec SRPM URL: http://people.redhat.com/~rdecarva/distorm/distorm-3.3-1.fc18.src.rpm Description: A lightweight, easy-to-use and fast disassembler/decomposer library for x86/AMD64. A decomposer means that you get a binary structure that describes an instruction rather than textual representation. Fedora Account System Username: rcvalle
Okay, we're talking past eachother. Severely. :-/ I've suggested you use %{_includedir}/distorm/ and instead you've used %{_includedir}/distorm/* That doesn't include the distorm directory: https://fedoraproject.org/wiki/Packaging:UnownedDirectories I've pointed out the package doesn't use Fedora's compiler flags yet, you've ignored that without a comment. Same for the base package dependency. Same for the unversioned docdirs feature. > rpmlint > > Is there any action I should take? Run rpmlint (or rpmlint -i for more helpful output) on the src.rpm and all built rpms. Feel free to ignore obvious false positives in the report, but fix anything else. Preferably add a comment here about whether/when you think what rpmlint reports is correct or incorrect. https://fedoraproject.org/wiki/Common_Rpmlint_issues It seems we also don't use the same terminology. For example, I've pointed out something about the library SONAME, and you modify the src.rpm name and spec file name instead. Well, no objections about that. It's okay to name the package "distorm" as the upstream project. But why don't you answer my questions? How to link with the library? Where is libdistorm3.so? And once more, why is the dynamic library built with DISTORM_STATIC and not DISTORM_DYNAMIC? The documentation you've linked, says: | Eventually, for compiling diStorm as a static library, | make sure DISTORM_STATIC is defined. For compiling diStorm | as a dynamic library or shared object, make sure DISTORM_DYNAMIC | is defined. What am I missing?
(In reply to Michael Schwendt from comment #30) > Okay, we're talking past eachother. Severely. :-/ > > I've suggested you use > %{_includedir}/distorm/ > and instead you've used > %{_includedir}/distorm/* I just fixed this. > > That doesn't include the distorm directory: > https://fedoraproject.org/wiki/Packaging:UnownedDirectories > > > I've pointed out the package doesn't use Fedora's compiler flags yet, you've > ignored that without a comment. I added optflags and set seemingly not working _hardened_build macro as well. > > Same for the base package dependency. The devel package already requires the base package. Should I also add "Requires: %{name}%{?_isa} = %{version}-%{release}"? Can you explain? > Same for the unversioned docdirs feature. It seems rpmbuild already has support for this. Can you elaborate? > > > > rpmlint > > > > Is there any action I should take? > > Run rpmlint (or rpmlint -i for more helpful output) on the src.rpm and all > built rpms. Feel free to ignore obvious false positives in the report, but > fix > anything else. Preferably add a comment here about whether/when you think > what > rpmlint reports is correct or incorrect. > https://fedoraproject.org/wiki/Common_Rpmlint_issues I'll go over it on the next iteration. > > > It seems we also don't use the same terminology. For example, I've pointed > out something about the library SONAME, and you modify the src.rpm name and > spec file name instead. Well, no objections about that. It's okay to name > the package "distorm" as the upstream project. But why don't you answer my > questions? How to link with the library? Where is libdistorm3.so? You do have complained about the package name in #c19. It's not libdistorm3.so.3.3 anymore, the major version was removed from the soname as you complained about it as well in #c27, it's libdistorm.so.3.3. I've also added a symlink to libdistorm.so.3.3 in base package to devel package named libdistorm.so as in https://fedoraproject.org/wiki/Packaging:Guidelines#Devel_Packages. If it's incorrect, how about explaining why it is incorrect and how to fix it instead of making rhetorical questions? > > > And once more, why is the dynamic library built with DISTORM_STATIC and not > DISTORM_DYNAMIC? > > The documentation you've linked, says: > > | Eventually, for compiling diStorm as a static library, > | make sure DISTORM_STATIC is defined. For compiling diStorm > | as a dynamic library or shared object, make sure DISTORM_DYNAMIC > | is defined. > > What am I missing? I overlooked this. Thanks for pointing it out. I just changed it by adding a separate patch.
> set seemingly not working _hardened_build macro as well. Why would you say that? It certainly appears in the build.log. > Should I also add "Requires: %{name}%{?_isa} = %{version}-%{release}"? Yes, as the guidelines say. > Can you explain? It is explained at https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package The reason we make dependencies arch-specific is that it helps package dependency resolvers. Perhaps you've seen Yum pulling in i686 packages on x86_64 in an attempt at trying to resolve non-arch-specific dependencies in a broken-dependency scenario. > > Same for the unversioned docdirs feature. > It seems rpmbuild already has support for this. Can you elaborate? You don't use %doc. You install manually into a versioned docdir, so you don't benefit from the F20 UnversionedDocdirs feature. And you introduce an unowned docdir directory, btw. The empty %doc lines in your spec file are superfluous. They don't do anything. Typically, one would delete unused lines in the spec file. > You do have complained about the package name in #c19. In January. It's taken some time before you would reply to that. ;-) > It's not libdistorm3.so.3.3 anymore, In the differently packaged openSUSE package, it's libdistorm3.so, albeit inside the private Python module directory. The upstream zip archive also contains a "libdistorm3.so" target. I don't complain, I mostly point out things. Perhaps you are in contact with upstream maintainers? Dunno. When deciding on a library SONAME and version, ABI/API stability/compatibility come into play. I don't know what is being planned. Hence the comment that a full ".3.3" version in the lib soname would be very strict, but I don't know the versioning scheme that will be used _downstream_ in the Fedora package. Have you planned packaging anything that links with this library? > I've also added a symlink to libdistorm.so.3.3 in base package > to devel package named libdistorm.so > If it's incorrect, how about explaining why it is incorrect and > how to fix it Well, rpmlint warns about "no-soname" since comment 1 of this ticket, but that and my comments about it have been ignored so far. The linked Common_Rpmlint_issues page in the Wiki explains a bit. Have you tried installing your builds yet? It doesn't work. $ rpm -qpR distorm-devel-3.3-1.fc19.x86_64.rpm|grep ^lib libdistorm.so()(64bit) $ rpm -qp --provides distorm-3.3-1.fc19.x86_64.rpm |grep ^lib libdistorm.so.3.3()(64bit) If you're really stuck at this point, I might give it a closer look. > instead of making rhetorical questions? ???
(In reply to Michael Schwendt from comment #32) > > set seemingly not working _hardened_build macro as well. > > Why would you say that? It certainly appears in the build.log. I had to set -fPIC manually, but it might be because I'm using F18. This may work on F19. > > > > Should I also add "Requires: %{name}%{?_isa} = %{version}-%{release}"? > > Yes, as the guidelines say. I just added it as well. > > > > Can you explain? > > It is explained at > https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package > > The reason we make dependencies arch-specific is that it helps package > dependency resolvers. Perhaps you've seen Yum pulling in i686 packages on > x86_64 in an attempt at trying to resolve non-arch-specific dependencies in > a broken-dependency scenario. Thanks for the explanation! > > > > > Same for the unversioned docdirs feature. > > It seems rpmbuild already has support for this. Can you elaborate? > > You don't use %doc. You install manually into a versioned docdir, so you > don't benefit from the F20 UnversionedDocdirs feature. And you introduce an > unowned docdir directory, btw. I just added it as well. > > The empty %doc lines in your spec file are superfluous. They don't do > anything. Typically, one would delete unused lines in the spec file. I just removed them. > > > > You do have complained about the package name in #c19. > > In January. It's taken some time before you would reply to that. ;-) But you do complained. > > > > It's not libdistorm3.so.3.3 anymore, > > In the differently packaged openSUSE package, it's libdistorm3.so, albeit > inside the private Python module directory. The upstream zip archive also > contains a "libdistorm3.so" target. Stop referring to the openSUSE package, these are the Python bindings with the shared library embedded for its use, not the library itself as a system-wide shared library. > > I don't complain, I mostly point out things. Perhaps you are in contact with > upstream maintainers? Dunno. When deciding on a library SONAME and version, > ABI/API stability/compatibility come into play. I don't know what is being > planned. Hence the comment that a full ".3.3" version in the lib soname > would be very strict, but I don't know the versioning scheme that will be > used _downstream_ in the Fedora package. I'm not in contact with maintainers, but from what I know from the project, it doesn't break API between minor releases and also have maintained compatibility between major releases. I just changed the name of the package again and changed the soname to have just the major version. I think this should suffice. > > Have you planned packaging anything that links with this library? Yes, I'm planning to submit another package that depends on this. It's a Ruby gem that uses this library through FFI. > > > > I've also added a symlink to libdistorm.so.3.3 in base package > > to devel package named libdistorm.so > > > If it's incorrect, how about explaining why it is incorrect and > > how to fix it > > Well, rpmlint warns about "no-soname" since comment 1 of this ticket, but > that and my comments about it have been ignored so far. The linked > Common_Rpmlint_issues page in the Wiki explains a bit. It was fixed, then regressed when the optflags was added and CFLAGS reset. I've removed the unnecessary patches and set all CFLAGS directly in the SPEC file. You really haven't saw the distorm.set-soname-field.patch all this time? > > Have you tried installing your builds yet? > It doesn't work. > > $ rpm -qpR distorm-devel-3.3-1.fc19.x86_64.rpm|grep ^lib > libdistorm.so()(64bit) > $ rpm -qp --provides distorm-3.3-1.fc19.x86_64.rpm |grep ^lib > libdistorm.so.3.3()(64bit) > > If you're really stuck at this point, I might give it a closer look. Looks good to me now: [rcvalle@ThinkPad ~]$ rpm -qp --requires /home/rcvalle/rpmbuild/RPMS/x86_64/distorm3-devel-3.3-1.fc18.x86_64.rpm | grep distorm distorm3 = 3.3-1.fc18 distorm3(x86-64) = 3.3-1.fc18 [rcvalle@ThinkPad ~]$ rpm -qp --provides /home/rcvalle/rpmbuild/RPMS/x86_64/distorm3-3.3-1.fc18.x86_64.rpm | grep distorm distorm3 = 3.3-1.fc18 distorm3(x86-64) = 3.3-1.fc18 libdistorm3.so.3()(64bit)
Spec URL: http://people.redhat.com/~rdecarva/distorm3/distorm3.spec SRPM URL: http://people.redhat.com/~rdecarva/distorm3/distorm3-3.3-1.fc18.src.rpm Description: A lightweight, easy-to-use and fast disassembler/decomposer library for x86/AMD64. A decomposer means that you get a binary structure that describes an instruction rather than textual representation. Fedora Account System Username: rcvalle
> $ rpm -qp --requires /home/rcvalle/rpmbuild/RPMS/x86_64/distorm3-devel-3.3-1.fc18.x86_64.rpm | grep distorm > distorm3 = 3.3-1.fc18 > distorm3(x86-64) = 3.3-1.fc18 That output is an indication that there's something wrong with the -devel package. Typically, shared lib -devel packages depend on the library SONAME. A dependency added for the .so symlink. I guess it's broken, but I haven't checked the new package yet.
Hi Michael, Do you have any updates on this?
Created attachment 792316 [details] remaining spec fixes * drop wrong usage of %_pkgdocdir, because it doesn't work in Fedora 20 if file is installed to %{_docdir}/%{name}-%{version} always * include license file COPYING via %doc magic * don't end %summary with a dot * omit leading article in %summary * drop superfluous %_isa-less base package dep from -devel package * remove superfluous stuff from %prep section * don't use %install -c (what does -c do?) * use %install -p to preserve permissions for prebuilt files * fix .so symlink in -devel package * fix bogus date in %changelog
I just updated the submission with your changes. Thanks! What are the next steps? (In reply to Michael Schwendt from comment #37) > Created attachment 792316 [details] > remaining spec fixes > > * drop wrong usage of %_pkgdocdir, because it doesn't work in Fedora 20 if > file is installed to %{_docdir}/%{name}-%{version} always > * include license file COPYING via %doc magic > * don't end %summary with a dot > * omit leading article in %summary > * drop superfluous %_isa-less base package dep from -devel package > * remove superfluous stuff from %prep section > * don't use %install -c (what does -c do?) > * use %install -p to preserve permissions for prebuilt files > * fix .so symlink in -devel package > * fix bogus date in %changelog
> What are the next steps? comment 25 With the bunch of fixes applied, the package should pass review. Except for the package name, which has been changed back and forth several times already. See comment 19 where I expressed my interpretation of the guidelines: | | There is nothing that mandates adding the "lib" prefix or appending | the "3". Upstream name is just "distorm". I'm waiting for the FPC to announce something related to the package naming guidelines clarification request in fpc trac ticket 336.
Why is this package not in the Fedora repository? The package `volatility` requires this package, which is already available in the Fedora repository. Because I needed the package `volatility` I also created a quick and dirty spec file [1], which builds and runs fine on my system. Without the distorm library, `volatility` doesn't work. The spec file [2] provided in this Bugzilla ticket isn't available anymore. Can I take over this package if the original author of this package-review request lost interest? Of course, I will improve my spec file further. [1] https://fedorapeople.org/cgit/keesdejong/public_git/rpmbuild.git/tree/SPECS/distorm.spec?id=32f3ac4d5b7696875be4409aa7b43296303e8157 [2] https://people.redhat.com/~rdecarva/libdistorm/libdistorm.spec
Mark it ar dead review and open a new bug for your review
This is an automatic check from review-stats script. This review request ticket hasn't been updated for some time. We're sorry it is taking so long. If you're still interested in packaging this software into Fedora repositories, please respond to this comment clearing the NEEDINFO flag. You may want to update the specfile and the src.rpm to the latest version available and to propose a review swap on Fedora devel mailing list to increase chances to have your package reviewed. If this is your first package and you need a sponsor, you may want to post some informal reviews. Read more at https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group. Without any reply, this request will shortly be considered abandoned and will be closed. Thank you for your patience.
This is an automatic action taken by review-stats script. The ticket submitter failed to clear the NEEDINFO flag in a month. As per https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews we consider this ticket as DEADREVIEW and proceed to close it.