Bug 993324 - Review Request: ell - Embedded LL library
Summary: Review Request: ell - Embedded LL library
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Michael Schwendt
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2013-08-05 19:32 UTC by Jonathan De Wachter
Modified: 2013-10-20 10:45 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2013-10-20 10:45:22 UTC
bugs.michael: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)
final spec file modifications (1.26 KB, patch)
2013-08-12 16:27 UTC, Michael Schwendt
no flags Details | Diff

Description Jonathan De Wachter 2013-08-05 19:32:25 UTC
Spec URL: http://sonkun.fedorapeople.org/ELL/ell.spec
SRPM URL: http://sonkun.fedorapeople.org/ELL/ell-0.0.0-20130617svn.src.rpm
Description: Embedded LL library is pure-header library to write EBNF grammars as C++ code. It eases the development of parser or similar applications, while removing the need to write a lexer.
Fedora Account System Username: sonkun

Packaging ELL will allow me to package SFGUI. As soon as ELL is in, I'll submit the SFGUI package.

As the description says, ELL is a header-only library which means extra care when reviewing its SPEC file. I asked some questions on IRC (#fedora-devel) regarding this special case and someone linked me to: https://bugzilla.redhat.com/show_bug.cgi?id=787510 . That's why I based my code on the GLM library's SPEC file. To ease your task, I uploaded it on my hosting page: http://sonkun.fedorapeople.org/GLM/glm.spec

Also, should I mention it's a snapshot package because I sent a mail to the upstream asking if he could add a tag for a pre-release but he hasn't yet replied.

Here's the koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=5780810

This is my first package and I'm seeking for a sponsor.

Comment 1 Michael Schwendt 2013-08-05 22:51:39 UTC
> extra care when reviewing its SPEC file

Not really.  Header-only C/C++ APIs aren't so special. It's just that there isn't much in the packaging guidelines. Also, there have been package submissions that named the src.rpm "something-devel" because there would be no base package to build.
There are more packaging pitfalls with normal lib+header APIs (e.g. sometimes there are headers with a missing lib, or a lib gets deleted after building it, or private headers get installed accidentally and packaged).


> Version:        0.0.0
> Release:        20130617svn

That's not one of Fedora's package versioning schemes yet:
https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Package_Versioning

Why the triple '0'? Why not a single '0'?

And for snapshots, you'll need the pre-release prefix (for much more flexibility):
https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Pre-Release_packages

  Release:        0.1.20130617svn


> Summary:        Embedded LL library

Sum up what it does, not what it's named. The %description is short enough to be suitable as a %summary. Example:

  Summary: Header-only C++ library to write EBNF grammars

https://fedoraproject.org/wiki/Examples_of_good_package_summaries
(not perfect but a good start)


> License:        LGPLv3

The source files say "or later" -> LGPLv3+

https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#.22or_later_version.22_licenses


> %build
> make %{?_smp_mflags}

Since this builds only the test-suite, this "make" invocation could/should be moved into the %check section.

It doesn't use Fedora's compiler flags, btw:
https://fedoraproject.org/wiki/Packaging:Guidelines#Compiler_flags


> # to track the usage of this library
> Provides:       %{name}-static = %{version}-%{release}

Yes, that is not in the guidelines and only helps with tracking (via scripts or repoquery). Basically, any other package can "BuildRequires: ell-devel" and build successfully without any need to make use of to the ell-static Provides. It's the responsibility of the package maintainer(s) to know when to rebuild dependencies against important ELL updates.


> Processing files: ell-devel-0.0.0-20130617svn.x86_64

It would be interesting to mention in the spec file that this package does not
set "BuildArch: noarch". I assume that is done, so the test-suite will be run for all target archs.

Comment 2 Christopher Meng 2013-08-06 00:40:05 UTC
Well, reporter, can you stop using 0.0.0 as version? 0.0 or =.= ;)

I don't think this is a good version for release, if you don't have a stable release, I don't think it's time to package it.

IMO you shouldn't add subpackage, you should add a virtual provide "Provides:  %{name}-devel".

Please fix things as Michael said and then we can review again.

Comment 3 Michael Schwendt 2013-08-06 08:16:56 UTC
> IMO you shouldn't add subpackage

Why not?

Comment 4 Christopher Meng 2013-08-06 08:21:19 UTC
We can keep them in main package I think. And just add virtual provides.

Comment 5 Michael Schwendt 2013-08-06 09:08:42 UTC
> We can

Ah, that's the connection between "can" => "shouldn't".

Yes, it would be possible to not build "ell-devel" but just "ell". There are no naming guidelines about it. The FPC considers it okay when an implicitly development related package doesn't add "-devel" to its name, if there is no need for a package split between run-time and build-time files. E.g. the package of a compiler or similar tool may contain headers (and even libs) without becoming a -devel package.

Whether that is good and convenient also for pure API packages is debatable. The thousands of -devel packages in the collection are considered fully optional and unnecessary at run-time, so one may remove them from an installation for various reasons. So far, it has been possible to use tools like rpmdev-rmdevelrpms or simple RPM queries to do that. The guidelines even comment on that with regard to removing all developer-related packages in an attempt at finding missing build requirements. It's helpful if header-only APIs are stored in -devel packages, too.


> And just add virtual provides.

For a non-virtual package name, they are automatic and versioned and even use %_isa. Adding them manually complicates matters. Introducing virtual package names bears a risk, too. Packagers could choose _either_ name to depend on. One packager will use ell-devel out of consistency with other build requirements, another will not even notice the virtual Provides and use "ell". And if you wanted to submit a query on BuildRequires, you would need to consider all available package names. Less convenient.

Comment 6 Jonathan De Wachter 2013-08-06 19:18:19 UTC
> Why the triple '0'? Why not a single '0'?

I thought explicit would be better than implicit, that's now fixed :)


> I don't think this is a good version for release, if you don't have a stable release, I don't think it's time to package it.

Actually, this came from a need to package SFGUI (http://sfgui.sfml-dev.de/) which has external dependencies.


Anyways, thank you for reviewing my package :) Here's the changes I made...

- Fixed the versioning schemes.
- Fixed version to contain a single 0.
- Added a comment to clarify the absence of "BuildArch: noarch"
- It now uses the Fedora's compiler flags.
- Fixed the license
- Moved make command to %check section
- Fixed release tag
- Fixed summary

... and its new URLs:

New koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=5786446
New Spec URL: http://sonkun.fedorapeople.org/ELL/ell.spec
New SRPM URL: http://sonkun.fedorapeople.org/ELL/ell-0-0.1.20130617svn.src.rpm

Comment 7 Michael Schwendt 2013-08-07 20:45:20 UTC
Another header-only API has come up, metslib in bug 993456, and there I thought, why not set "BuildArch: noarch" for the -devel subpackage?

What do you think?

Comment 8 Jonathan De Wachter 2013-08-08 23:16:25 UTC
> Another header-only API has come up, metslib in bug 993456, and there I
> thought, why not set "BuildArch: noarch" for the -devel subpackage?
> 
> What do you think?

In the absolute, the packaging guidelines should think for me, shouldn't they :p I couldn't find anything mentioning we can set package-specific BuildArch tag. I thought it was a global statement. Tell me what to do and I'll do it.

I don't want to hurry but is my package accepted ? Because its submission has two goals, first joining the 'packager' group which will allow me to take over the SFML package (I'm its upstream and I have its current maintainer agreement). I also have its 5 bindings packaged on my computer and I'm looking forward to sending them to the Fedora repository. The second goal is to package SFGUI which uses ell library.

Or at worst, can I have a hint regarding how long it's gonna take ? So I could give deadlines to the SFML community.

Thank you.

Comment 9 Michael Schwendt 2013-08-09 07:10:19 UTC
Well, the packaging guidelines don't try to document what RPM can do, but how to package things.

And yes, "BuildArch: noarch" can be set per sub-package (it used to be impossible years ago), with the added responsibility that you don't include any arch-specific packages in such a noarch package accidentally. [A popular case of noarch sub-packages is huge documentation that is split off, btw.]

> I don't want to hurry but is my package accepted ?

I'll take another look at set the flags as per:
https://fedoraproject.org/wiki/Package_Review_Process

Comment 10 Michael Schwendt 2013-08-09 07:26:42 UTC
> don't include any arch-specific packages in such

typo:  s/packages/files/

Comment 11 Michael Schwendt 2013-08-12 16:27:56 UTC
Created attachment 785775 [details]
final spec file modifications

These are the last modifications I would suggest for the spec file:

 * the noarch -devel package
 * an empty %build section, albeit only to silence rpmlint
 * the dist tag:
   https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Using_the_.25.7B.3Fdist.7D_Tag
 * preserved time stamps for copied files:
   https://fedoraproject.org/wiki/Packaging:Guidelines#Timestamps
 * a trailing slash for the included directory tree in %files list (just for increased readability - not mandatory - also see further below)

[...]

With regard to general packaging quality, it would also be an idea to be more explicit about which files to include. For example,

  %dir %{_includedir}/ell
  %{_includedir}/ell/*.h

would make the build fail, if no header files are found, whereas with the current spec,

  %{_includedir}/ell/

even an empty directory would result in a successful build. Alternatively, one can add safety/sanity checks to the %check section, too.

I just point this out, because it's a single line in the %install section that copies the headers dir -> a single point of failure.

[...]

The remaining changes to the package may be applied in Fedora package git, so no need to make available another revision here in the review ticket.

APPROVED

Comment 12 Jonathan De Wachter 2013-08-13 03:09:27 UTC
New Package SCM Request
=======================
Package Name: ell
Short Description: Header-only C++ library to write EBNF grammars
Owners: sonkun
Branches: f18 f19
InitialCC: mschwendt echevemaster

Comment 13 Gwyn Ciesla 2013-08-13 12:43:28 UTC
fedora-review flag not set to '+'

Comment 14 Michael Schwendt 2013-08-13 13:41:54 UTC
Sigh. Bugzilla once again managed to confuse me with its "review" flag it offers when adding an attachment. Obviously, that's not the "fedora-review" flag.

Comment 15 Gwyn Ciesla 2013-08-13 14:58:54 UTC
Git done (by process-git-requests).

Comment 16 Fedora Update System 2013-08-14 10:22:30 UTC
ell-0-0.2.20130617svn.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/ell-0-0.2.20130617svn.fc19

Comment 17 Fedora Update System 2013-08-14 10:26:45 UTC
ell-0-0.2.20130617svn.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/ell-0-0.2.20130617svn.fc18

Comment 18 Fedora Update System 2013-08-15 02:50:38 UTC
ell-0-0.2.20130617svn.fc19 has been pushed to the Fedora 19 testing repository.

Comment 19 Christopher Meng 2013-09-25 14:48:22 UTC
2013-10-15 is Beta freeze, please push them ASAP.

Comment 20 Christopher Meng 2013-10-20 02:40:11 UTC
What are you doing?

Comment 21 Michael Schwendt 2013-10-20 10:45:22 UTC
Christopher, your inflationary usage of the needinfo flag is getting ridiculous. If you request needinfo, please ask specific questions. Be verbose.

Btw, there is no need to push anything "ASAP" here. The package is included with Rawhide and Fedora 20 already since August 14th, so I'm closing this bug.


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