https://bugzilla.redhat.com/show_bug.cgi?id=1182261 --- Comment #5 from Dodji Seketeli <dodji@xxxxxxxxxxxx> --- Created attachment 980401 --> https://bugzilla.redhat.com/attachment.cgi?id=980401&action=edit First iteration on proposed changes to the libabigail package Hey Sinny, As Richard Shaw said in his comment, this looks like great work. Thank you for that! I also agree with his comments and I am attaching this patch that addresses his comments. Namely: > 1. It's preference, not required, but it's a good idea to list your > BuildRequires on separate lines. About the only time I list > multiple is if they are closely related (autoconf, automake) or > let's say you have something that manipulates images then I could > see having all the image devel libraries on one line. > I have updated the .spec file in the patch to address this. > 2. Requires from the devel package should be arch specific, i.e.: > Requires: %{name} = %{version}-%{release} > to: > Requires: %{name}%{?_isa} = %{version}-%{release} I have made this change too. > 3. In install since your not doing anything out of the ordinary you > could use the "%make_install" macro. I have made this change. And I have also added a call to the 'install-man-and-info-doc' target of the Makefile to install the man pages and info doc. > 4. In %files unless your using non-default permissions you don't need > %defattr. Fixed. > > 5. Typically %doc is right under %files. It's not wrong to > do so but it's unusual. Fixed. I am also addressing the comments from Rahul Sundaram: > Libtool archives, foo.la files, should not be included Removed. > Do you need "autoreconf -i"? If the source code tarball was made using make dist, then you are right, "autoreconf -i" is not needed. But in the changes I am proposing here, I am falling back to just using "tar" to build the source tarball from the git snapshot, so that the person who is actually constructing the tarball doesn't need to have the autotools installed just to re-construct the tarball. And in that case, yes the "autoreconf -i" is needed. So I am keeping it in my proposed changes. In addition to this, I am proposing the changes below to address some more nits: * As this packages a pre-release snapshot from git, specify the git commit in the Release TAG, as per http://fedoraproject.org/wiki/Packaging:NamingGuidelines#Snapshot_packages. * In the comments of the Source0 tag, give more details about how to get that specific git commit from the upstream git repository. Also, just use a plain 'tar' command to construct the tarball, rather than requiring that the user runs autoreconf -i and then configure and then make dist. I think it's nice to ease the work of whoever is going to be getting that source tarball. * Be more factual in the %description of libabigail: introduce the actual command line tools that the package contains, a bit like what the diffutils package does. The libabigail tools are not unliked the diffutils tools, but for the ABI of shared libraries; that is why I think it's interesting to take their wording as an example. Also, do not introduce the library itself in the %description of the libabigail package, because the library is not what the end-user installing the libabigail package would be actually *using*. I'd rather introduce the library in the %description of the *libabigail-devel* package. * Rename the libabigail-man sub-package into libabigail-doc and move all the documentation there, as there is quite a lot of documentation now (man, texinfo, apidoc, html manual). This is as per http://fedoraproject.org/wiki/Packaging:Guidelines#Documentation which says: "if there's a lot of documentation, consider putting it into a subpackage. In this case, it is recommended to use *-doc as the subpackage name". * Make sure the zip-archive feature is disabled as it can be automatically turned on if the libzip library is present at build time; and this feature is going to be deprecated in upstream. * Add texinfo documentation generation at build time. * Specifically install man page and texinfo documentation using the new upstream "install-man-and-info-doc" make target. For the texinfo pages, add post (un)install scriptlets to update the global texinfo directory accordingly. * If make check fails, cat tests/test-suite.log so that we can see what the details of the failings are in the output log of the build. This is extremely handy to spot and fix causes of test suite errors that happen when the package is built remotely on koji. * Explicitly list the binary tools and libraries that are included in the package so that the person who updates the package can see when new upstream binaries are added and then choose to add them to the package or not. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review