Bug 1275888

Hey Chris,

So I'm not picking up and doing a formal review/sponsorship, but I took a look at your spec and there are a few things I'd like to point out:

* Your summary confuses me. Where does the "bad intentions" come in? It doesn't sound good. Please reword it in a more useful manner. Likewise, the "bad intentions" probably should be stricken from the description too.

* Please use "Source0" instead of "Source". It makes it much easier to deal with if you ever have to have more sources, and it's just a good practice to have. Also note it's a good idea to not use "Patch" and instead "Patch0" for declaring a patch as well. When attached with a number, you can keep incrementing it as you add more of them.

* Your BuildRequires, while "legal", are difficult to read as it is all in one line. The currently preferred style is to put each BuildRequire on its own line, but I would suggest that at the minimum to chunk them up such that you have three or four per BuildRequire line.

* I am impressed you used the pkgconfig() virtual provides, as not many people use them. Kudos! However, as these can get rather long, it is usually recommended that these are split out into one per line, simply for readability purposes. It's certainly not required, merely a suggestion.

* If you are targeting exclusively Fedora, I'd strongly recommend replacing "make %{?_smp_mflags}" with the cleaner "%make_build", as it is the build counterpart to the "%make_install" macro you use now. If you target anything older than EL7, you're going to need to rip out the %make_install macro and replace it with "make DESTDIR=%{buildroot} install".

* Your two sed operations should have a comment explaining why you are doing this. Even better, if you produce a patch that could be upstreamed, then you could maintain the changed behavior as a patchset that could be dropped in future updates of the software.

I also forgot to mention one more thing: if you are intending to target older than EL7, you also need to replace "%autosetup" with "%setup -q -n", as the former was introduced in EL7.

Erk, I meant replace it with "%setup -q", as the "-n" is unnecessary.

Okay, I just updated everything to be more in line with what Neal suggested (thanks for the help!).

dutchipoo's scratch build of balde-0.1.2-1.fc22.src.rpm for f23 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=11614801

Dutch,

After looking over your spec again, I'm wondering why you're pointing to an Amazon AWS URL for Source0, when in fact you could use the GitHub one? The following is perfectly valid:

Source0: https://github.com/balde/balde/releases/download/v%{version}/%{name}-%{version}.tar.xz

Other than that, it looks great!

Can't fully understand the excitement about the packaging so far.


* While the pkgconfig BuildRequires are mentioned in the guidelines, they don't add much value compared with specifying the needed packages in BuildRequires directly. Afterall, if the configure script really runs pkg-config queries to look for stuff, the .pc files must be present. If the BuildRequires were incorrect, the build would fail early which is not a big issue.


* It's surprising that the spec file does not contain any %changelog section yet: https://fedoraproject.org/wiki/Packaging:Guidelines#Changelogs


* Consider running "rpmlint -i" on all (!) built packages, i.e. including the src.rpm package file.


> balde-0.1.2.spec

You *really* do not want to rename the spec file for each version upgrade. Certainly not within dist git.

It must be named "balde.spec" only:

  https://fedoraproject.org/wiki/Packaging:Guidelines#Spec_File_Naming

Perhaps you use a single directory for all your package versions of balde. You may want to adjust your RPM macros to set up a unique source directory per %version.


> Name: balde
> Group: Development/Libraries

The Group tag for base runtime library packages has been "System Environment/Libraries" for many years.


> BuildRequires: autoconf
> BuildRequires: automake
> BuildRequires: libtool

None of these are needed.


> BuildRequires: doxygen

No doxygen generated docs are included, though.

$ rpm -qpd balde-devel-0.1.2-1.fc23.x86_64.rpm 
$ rpm -qpd balde-0.1.2-1.fc23.x86_64.rpm 
/usr/share/doc/balde/COPYING
/usr/share/doc/balde/README.md
$


> %package devel

https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package


>   CC     libbalde_la-app.lo
>   CC     libbalde_la-exceptions.lo
>   CC     libbalde_la-cgi.lo
>   CC     libbalde_la-resources.lo
>   CC     libbalde_la-routing.lo

Enable verbose build output with

  V=1 %make_build

so you get to see the used compiler/linker flags actually, which can be a life-saver in case of problems when wrong flags and/or paths have been used during building.

https://fedoraproject.org/wiki/Packaging:Guidelines#Compiler_flags


> tests

https://fedoraproject.org/wiki/Packaging:Guidelines#Test_Suites


> $ grep ^Req balde.pc 
> Requires: glib-2.0 >= 2.34, gio-2.0 >= 2.34, shared-mime-info

While the dep on glib and gio is plausible (balde headers include glib/gio headers), the shared-mine-info dep is superfluous and very likely an artifact of squeezing it into the @GLIB_DEP@ configure macro.


> %doc COPYING README.md

https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text


> %{_bindir}/balde-template-gen

It belongs into the -devel package, doesn't it?


> %exclude %{_libdir}/libbalde.la
> %exclude %{_libdir}/libbalde_template.la

Caution! Prefer "rm -f" in %install for all files you *really* do not want to include in any (sub-)package. Why? %exclude doesn't remove those files from the buildroot, but only from the list of files that must be included. It would still be possible to include them somewhere accidentally. And if "make install" did not install these libtool archives, %exclude would cause the build to fail, whereas "rm -f" would not. So, cleaning up the buildroot with "rm" is safer and hence superior.

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.