Bug 1310793 - Review Request: unqlite - Transactional Embedded Database Engine
Review Request: unqlite - Transactional Embedded Database Engine
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: 2016-02-22 11:34 EST by Paulo Henrique Rodrigues Pinheiro
Modified: 2016-03-09 15:59 EST (History)
2 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 Paulo Henrique Rodrigues Pinheiro 2016-02-22 11:34:01 EST
Spec URL: https://raw.githubusercontent.com/paulohrpinheiro/fedora-rpm-unqlite/master/unqlite.spec
SRPM URL: https://github.com/paulohrpinheiro/fedora-rpm-unqlite/raw/master/unqlite-1.1.6-1.fc23.src.rpm
Description: 

Hello!

I just pack the unqlite library and would like you to execute one review to add it to the Fedora packages. Already there are bindings for rust, perl and python that always include the source code for the library in these packages.

From http://unqlite.org:

"UnQLite is a self-contained C library without dependency. It requires very minimal support from external libraries or from the operating system. This makes it well suited for use in embedded devices that lack the support infrastructure of a desktop computer. This also makes UnQLite appropriate for use within applications that need to run without modification on a wide variety of computers of varying configurations."

Thank you!

Fedora Account System Username: paulohrpinheiro
Comment 1 Paulo Henrique Rodrigues Pinheiro 2016-02-22 12:12:09 EST
And, please, this is my first package, and I need a sponsor. Thanks!
Comment 2 Richard Shaw 2016-02-22 13:25:02 EST
A quick spec review on my lunch break...

1. rpmbuild doesn't care but typically %{pre,post,postun,etc} would be after %install

2. In %build, why is the debug package being disabled? Typically this is only permitted for noarch packages where it doesn't produce anything useful.

3a. A license file only needs to be included in one package assuming the other requires it (in this case -devel requires the base package)

3b. License files should use the %license macro rather than %doc.

3c. Since a readme file is provided it should be included in the base package with %doc.

So all that to say the base package should be updated to:
%license license.txt
%doc README.md

4. Typo in the description at the beginning of the -devel package.
Comment 3 Paulo Henrique Rodrigues Pinheiro 2016-02-22 14:04:33 EST
(In reply to Richard Shaw from comment #2)
> A quick spec review on my lunch break...
> 

> 1. rpmbuild doesn't care but typically %{pre,post,postun,etc} would be after
> %install

OK


> 2. In %build, why is the debug package being disabled? Typically this is
> only permitted for noarch packages where it doesn't produce anything useful.

I followed the directions of the documentation* to see if it was a recurring problem, but could not make the debug package be generated.

* https://fedoraproject.org/wiki/Packaging:Debuginfo?rd=Packaging/Debuginfo

After this, to "silent" rpmlint, I put a strip in make install.


> 3a. A license file only needs to be included in one package assuming the
> other requires it (in this case -devel requires the base package)

OK, I put this because rpmlint give-me "no-documentation" :)

 
> 3b. License files should use the %license macro rather than %doc.
> 
> 3c. Since a readme file is provided it should be included in the base
> package with %doc.

There's no README.md, only license.txt...

 
> So all that to say the base package should be updated to:
> %license license.txt
> %doc README.md


Can I leave rpmlint with these warnings? My output:

[paulohrpinheiro@localhost SPECS]$ rpmlint unqlite.spec ../RPMS/x86_64/unqlite-*
unqlite.x86_64: W: no-documentation
unqlite-devel.x86_64: W: no-documentation
2 packages and 1 specfiles checked; 0 errors, 2 warnings.


> 4. Typo in the description at the beginning of the -devel package.
OK


Thank you. The files are updated in original links:

Spec URL: https://raw.githubusercontent.com/paulohrpinheiro/fedora-rpm-unqlite/master/unqlite.spec

SRPM URL: https://github.com/paulohrpinheiro/fedora-rpm-unqlite/raw/master/unqlite-1.1.6-1.fc23.src.rpm
Comment 4 Richard Shaw 2016-02-22 14:15:03 EST
(In reply to Paulo Henrique Rodrigues Pinheiro from comment #3)
> (In reply to Richard Shaw from comment #2)
> 
> > 2. In %build, why is the debug package being disabled? Typically this is
> > only permitted for noarch packages where it doesn't produce anything useful.
> 
> I followed the directions of the documentation* to see if it was a recurring
> problem, but could not make the debug package be generated.
> 
> * https://fedoraproject.org/wiki/Packaging:Debuginfo?rd=Packaging/Debuginfo
> 
> After this, to "silent" rpmlint, I put a strip in make install.

Not sure what is going on here, I removed the strip command and get a usable debuginfo package.


> > 3a. A license file only needs to be included in one package assuming the
> > other requires it (in this case -devel requires the base package)
> 
> OK, I put this because rpmlint give-me "no-documentation" :)

Yes, it's just warning you, since upstream doesn't provide any that's OK.


> > 3c. Since a readme file is provided it should be included in the base
> > package with %doc.
> 
> There's no README.md, only license.txt...

I just noticed that, I was looking at it in github where there is one. Has there just not been a release since it was added?


> Can I leave rpmlint with these warnings? My output:
> 
> [paulohrpinheiro@localhost SPECS]$ rpmlint unqlite.spec
> ../RPMS/x86_64/unqlite-*
> unqlite.x86_64: W: no-documentation
> unqlite-devel.x86_64: W: no-documentation
> 2 packages and 1 specfiles checked; 0 errors, 2 warnings.

Yes, that's OK.

Also, just playing around a bit with the Makefile, after removing the strip command I added a symbolic link to the library to go in the -devel package:

ln -rs $(LIBDIR)/$(LIBNAME) $(LIBDIR)/libunqlite.so

Then added to the spec:

%files devel
%{_includedir}/unqlite.h
%{_libdir}/libunqlite.so

This is typically needed as a project using this for a dependency wouldn't necessarily know about the soversion.
Comment 5 Paulo Henrique Rodrigues Pinheiro 2016-02-22 15:56:57 EST
(In reply to Richard Shaw from comment #4)
> (In reply to Paulo Henrique Rodrigues Pinheiro from comment #3)
> > (In reply to Richard Shaw from comment #2)
> (...)
> > After this, to "silent" rpmlint, I put a strip in make install.
> 
> Not sure what is going on here, I removed the strip command and get a usable
> debuginfo package.

Some issue in my config. In spec I put this line:

    %debug_package


And now I get the debuginfo package.



> > > 3c. Since a readme file is provided it should be included in the base
> > > package with %doc.
> > 
> > There's no README.md, only license.txt...
> 
> I just noticed that, I was looking at it in github where there is one. Has
> there just not been a release since it was added?

The stable version is only on site. The github is "just for fun", according upstream devs. New features are "frozzen" by now.


> Also, just playing around a bit with the Makefile, after removing the strip
> command I added a symbolic link to the library to go in the -devel package:
> 
> ln -rs $(LIBDIR)/$(LIBNAME) $(LIBDIR)/libunqlite.so
> 
> Then added to the spec:
> 
> %files devel
> %{_includedir}/unqlite.h
> %{_libdir}/libunqlite.so
> 
> This is typically needed as a project using this for a dependency wouldn't
> necessarily know about the soversion.

OK


Thanks, now it's good!
Comment 6 Richard Shaw 2016-02-22 16:19:19 EST
A couple of other things to note, the summary in this review request should match what's in the spec file.

The best "one-liner" I've seen is actually in the description of the github project, "Transactional Embedded Database Engine"

Also, when making updates per my (or anyone else's) feedback, it's customary to bump the release and add what you changed to the changelog. That also makes it easy when you update the SPEC and SRPM links as you can just paste the contents of the changelog here.
Comment 7 Paulo Henrique Rodrigues Pinheiro 2016-02-22 17:56:28 EST
(In reply to Richard Shaw from comment #6)
> A couple of other things to note, the summary in this review request should
> match what's in the spec file.
> 
> The best "one-liner" I've seen is actually in the description of the github
> project, "Transactional Embedded Database Engine"
> 
> Also, when making updates per my (or anyone else's) feedback, it's customary
> to bump the release and add what you changed to the changelog. That also
> makes it easy when you update the SPEC and SRPM links as you can just paste
> the contents of the changelog here.

I changed the package and ticket summary, an put all changes in changelog.

It is good, or can it be improved?

Thank you, the day is not over yet and I learned a lot already!!
Comment 8 Upstream Release Monitoring 2016-02-22 18:51:00 EST
paulohrpinheiro's scratch build of unqlite-1.1.6-4.fc23.src.rpm for f23 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=13099781
Comment 9 Richard Shaw 2016-02-25 08:44:22 EST
Paulo, can you post new SPEC and SRPM links?
Comment 10 Paulo Henrique Rodrigues Pinheiro 2016-02-25 11:24:18 EST
(In reply to Richard Shaw from comment #9)
> Paulo, can you post new SPEC and SRPM links?

Sorry, are these:

https://raw.githubusercontent.com/paulohrpinheiro/fedora-rpm-unqlite/master/unqlite.spec

https://github.com/paulohrpinheiro/fedora-rpm-unqlite/raw/master/unqlite-1.1.6-4.fc23.src.rpm
Comment 11 Richard Shaw 2016-03-09 15:59:05 EST
(In reply to Paulo Henrique Rodrigues Pinheiro from comment #5)
> (In reply to Richard Shaw from comment #4)
> > (In reply to Paulo Henrique Rodrigues Pinheiro from comment #3)
> > > (In reply to Richard Shaw from comment #2)
> > (...)
> > > After this, to "silent" rpmlint, I put a strip in make install.
> > 
> > Not sure what is going on here, I removed the strip command and get a usable
> > debuginfo package.
> 
> Some issue in my config. In spec I put this line:
> 
>     %debug_package
> 
> 
> And now I get the debuginfo package.

There might be something quirky in your setup. fedora-review doesn't like the explicit %debug_package because it's inserted automatically so it's complaining about the debuginfo package already existing.

I dug into the license patch and it appears to just be to convert line endings. Patching the license is generally not allowed but I'm having trouble finding where in the guidelines that is said.

What I usually do in this case is use dos2unix

BuildRequires: dos2unix
...
dos2unix license.txt

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