https://bugzilla.redhat.com/show_bug.cgi?id=1275888
Bug ID: 1275888 Summary: Review Request: balde - a glib web microframework Product: Fedora Version: rawhide Component: Package Review Severity: medium Assignee: nobody@fedoraproject.org Reporter: clamberson@gmail.com QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org
Spec URL: https://s3-us-west-2.amazonaws.com/dutch-packages/balde-0.1.2.spec SRPM URL: https://s3-us-west-2.amazonaws.com/dutch-packages/balde-0.1.2-1.fc22.src.rpm Description: This is balde, a microframework for C based on GLib. It is designed to be fast, simple, and memory efficient. Most of its architecture is based on other microframeworks, like Flask, and it can run on any web server that supports CGI and/or FastCGI. Fedora Account System Username: dutchipoo
This is my first package submission! It sounds like I'll be needing a sponsor to help me through the process. Thanks for any help. If it helps to know, I'm not the upstream developer; I just want to see this fairly stable software in Fedora. I built it fresh with mock to make sure the package worked from scratch. Let me know if you need anything else from me.
https://bugzilla.redhat.com/show_bug.cgi?id=1275888
Chris Lamberson clamberson@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |177841 (FE-NEEDSPONSOR)
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=177841 [Bug 177841] Tracker: Review requests from new Fedora packagers who need a sponsor
https://bugzilla.redhat.com/show_bug.cgi?id=1275888
Neal Gompa ngompa13@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |ngompa13@gmail.com
--- Comment #1 from Neal Gompa ngompa13@gmail.com --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=1275888
--- Comment #2 from Neal Gompa ngompa13@gmail.com --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=1275888
--- Comment #3 from Neal Gompa ngompa13@gmail.com --- Erk, I meant replace it with "%setup -q", as the "-n" is unnecessary.
https://bugzilla.redhat.com/show_bug.cgi?id=1275888
--- Comment #4 from Dutch Lamberson clamberson@gmail.com --- Okay, I just updated everything to be more in line with what Neal suggested (thanks for the help!).
https://bugzilla.redhat.com/show_bug.cgi?id=1275888
--- Comment #5 from Upstream Release Monitoring upstream-release-monitoring@fedoraproject.org --- dutchipoo's scratch build of balde-0.1.2-1.fc22.src.rpm for f23 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=11614801
https://bugzilla.redhat.com/show_bug.cgi?id=1275888
--- Comment #6 from Neal Gompa ngompa13@gmail.com --- 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%%7Bversion%7D/%%7Bname%7D...
Other than that, it looks great!
https://bugzilla.redhat.com/show_bug.cgi?id=1275888
--- Comment #7 from Michael Schwendt bugs.michael@gmx.net --- 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.
package-review@lists.fedoraproject.org
-
Red Hat Bugzilla