Bug 1275888 - Review Request: balde - a glib web microframework
Review Request: balde - a glib web microframework
Status: NEW
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Extras Quality Assurance
:
Depends On:
Blocks: FE-NEEDSPONSOR
  Show dependency treegraph
 
Reported: 2015-10-28 01:10 EDT by Dutch Lamberson
Modified: 2018-01-29 21:44 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed:
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Dutch Lamberson 2015-10-28 01:10:22 EDT
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.
Comment 1 Neal Gompa 2015-10-28 02:26:29 EDT
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.
Comment 2 Neal Gompa 2015-10-28 02:38:28 EDT
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.
Comment 3 Neal Gompa 2015-10-28 02:38:59 EDT
Erk, I meant replace it with "%setup -q", as the "-n" is unnecessary.
Comment 4 Dutch Lamberson 2015-10-28 13:07:36 EDT
Okay, I just updated everything to be more in line with what Neal suggested (thanks for the help!).
Comment 5 Upstream Release Monitoring 2015-10-28 13:13:58 EDT
dutchipoo's scratch build of balde-0.1.2-1.fc22.src.rpm for f23 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=11614801
Comment 6 Neal Gompa 2015-11-02 23:13:35 EST
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!
Comment 7 Michael Schwendt 2015-12-01 03:58:56 EST
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.

Note You need to log in before you can comment on or make changes to this bug.