Bug 999690

Hi,

I'd like to offer some review comments.

The obvious one here, is that I'm guessing you're seeking a preapproval of sorts - the major issue would be that a re-review would be required once a stable tag is packaged. In the meantime, rpmlint produces a few things to look at:

bcache-tools.x86_64: W: incoherent-version-in-changelog 20130820 ['0.0-1.20130820git.fc19', '0.0-1.20130820git']

I'm expecting a "1.0" release tag in future would resolve this, otherwise, please see the naming guidelines:
http://fedoraproject.org/wiki/Packaging:NamingGuidelines#Package_Versioning


bcache-tools.x86_64: W: only-non-binary-in-usr-lib
There are only non binary files in /usr/lib so they should be in /usr/share.

False positive?

bcache-tools.x86_64: W: no-manual-page-for-binary bcache-register
Each executable in standard binary directories should have a man page.
There are man pages for each executable but this one.

Other points:
Please use the -p flag on installation to preserve the timestamp as per:
http://fedoraproject.org/wiki/Packaging:Guidelines#Timestamps

The buildroot tag is obsolete and should be removed:
http://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag

Consider using %global over %define:
http://fedoraproject.org/wiki/Packaging:Guidelines#.25global_preferred_over_.25define

The %clean is obsolete:
http://fedoraproject.org/wiki/Packaging:Guidelines#.25clean

Hopefully this helps you towards a cleaner spec file for your stable revision.

You should fix the problems mentioned and upload a -2 release package before we can continue.

_udevrulesdir is defined among the RPM macros shipped by the systemd package. You should not define the macro yourself.

Thanks for the feedback. I (probably) fixed all issues, but:

- I changed package versioning, but the package being based on a git snapshot of pre-release may still ring alarms.
- W: only-non-binary-in-usr-lib. Must be False positive: /usr/lib/udev/rules.d/61-bcache.rules is part of the package. I guess moving that to /usr/share/udev/rules.d is out of the question.

New release available here:

Spec URL: http://bdsync.rolf-fokkens.nl/bcache-tools-20130822/bcache-tools-0-0.2.20130820git.fc19.src.rpm
SRPM URL: http://bdsync.rolf-fokkens.nl/bcache-tools-20130822/bcache-tools.spec

(In reply to Rolf Fokkens from comment #4)
> /usr/lib/udev/rules.d/61-bcache.rules is part of the package. I guess moving
> that to /usr/share/udev/rules.d is out of the question.

Exactly.

I think you need to BuildRequire systemd to ensure the macro is defined.

Processed Comment 5 (should have thought about that myself).

Spec URL: http://bdsync.rolf-fokkens.nl/bcache-tools-20130822.1/bcache-tools-0-0.3.20130820git.fc19.src.rpm
SRPM URL: http://bdsync.rolf-fokkens.nl/bcache-tools-20130822.1/bcache-tools.spec

> W: only-non-binary-in-usr-lib. Must be False positive:
> /usr/lib/udev/rules.d/61-bcache.rules is part of the package.

It would be a "false positive", if rpmlint did miss at least one binary-in-usr-lib. But rpmlint is correct, since the rules file is no binary. A RFE bug report for rpmlint could be opened to request that it excludes the udev rules dir from that check - http://bugz.fedoraproject.org/fedora-review - and then it would not warn about files like this anymore.

Not sure which rpmlint is used, the one on my F19 system wasn't as verbose as the one you're using. So I don't think I can open a proper RFE bug report.

Fixed the udev rules, it now works!

After following the instructions on https://wiki.archlinux.org/index.php/Bcache:
- /dev/bcache0 is automagically there
- /home can now automatically be mounted during boot.

SRPM URL: http://bdsync.rolf-fokkens.nl/bcache-tools-20130824/bcache-tools-0-0.4.20130820git.fc19.src.rpm
Spec URL: http://bdsync.rolf-fokkens.nl/bcache-tools-20130824/bcache-tools.spec

I'm not very familiar with udev though, so a review on the rules might be wise.

> Not sure which rpmlint is used, the one on my F19 system 

There is no different rpmlint for the other Fedora releases. Run rpmlint (or the more helpful rpmlint -i) on *both* the src.rpm and the built rpms. What do you get?

I've opened bug 1000723 <- http://bugz.fedoraproject.org/rpmlint

Thanks, that explains it, I was only using rpmlint on the src.rpm. I have better output now.

New release:
- moved bcache-register to /usr/lib/udev
- suppress bcache-register error output (caused by registering device twice)
- removed man page for bcache-register
- added bcache-status
- added tar and gcc to BuildRequires
- added python to Requires

SRPM URL: http://bdsync.rolf-fokkens.nl/bcache-tools-20130825/bcache-tools-0-0.5.20130820git.fc19.src.rpm
Spec URL: http://bdsync.rolf-fokkens.nl/bcache-tools-20130825/bcache-tools.spec

Just some comments, no full review:

> added tar and gcc to BuildRequires

Adding these two moves into the wrong direction.
https://fedoraproject.org/wiki/Packaging:Guidelines#Exceptions_2
[ https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRequires ]


* https://fedoraproject.org/wiki/Packaging:Guidelines#All_patches_should_have_an_upstream_bug_link_or_comment


> %defattr(-,root,root,-)

https://fedoraproject.org/wiki/Packaging:Guidelines#File_Permissions
This is really the default even for EL5.

Thanks, I processed your comments:

* Mon Aug 26 2013 Rolf Fokkens <rolf> - 0-0.6.20130820git
- removed tar and gcc from BuildRequires
- removed defattr from files section
- added upstream references to patches in comments 

SRPM URL: http://bdsync.rolf-fokkens.nl/bcache-tools-20130826/bcache-tools-0-0.6.20130820git.fc19.src.rpm
Spec URL: http://bdsync.rolf-fokkens.nl/bcache-tools-20130826/bcache-tools.spec

Head some very good interaction upstream:

http://thread.gmane.org/gmane.linux.kernel.bcache.devel/1948
http://thread.gmane.org/gmane.linux.kernel.bcache.devel/1947
http://thread.gmane.org/gmane.linux.kernel.bcache.devel/1946

So, another one:

* Mon Aug 26 2013 Rolf Fokkens <rolf> - 0-0.7.20130820git
- updated bcache-status to latest upstream gist
- removed the rules patch

SRPM URL: http://bdsync.rolf-fokkens.nl/bcache-tools-0-0.7.20130820git/bcache-tools-0-0.7.20130820git.fc19.src.rpm
Spec URL: http://bdsync.rolf-fokkens.nl/bcache-tools-0-0.7.20130820git/bcache-tools.spec

Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed

===== Issues =====

- URL tag should be: http://bcache.evilpiepirate.org/
- VCS tag can be: http://evilpiepirate.org/git/bcache-tools.git
- %global _udevlibdir /usr/lib/udev is very bad idea. Use %{_prefix}/lib/udev
- CFLAGS="%{optflags}" make .. is bad idea too. Use %configure before.
  In %prep use "echo '#!/bin/sh' > ./configure"
- Why you don't use make install (%make_install) ?
- I see in upstream tests are present, but you doesn't use it. Why?

===== MUST items =====

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[!]: %build honors applicable compiler flags or justifies otherwise.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Package contains desktop file if it is a GUI application.
[-]: Development files must be in a -devel package
[x]: Package requires other packages for directories it uses.
[x]: Package uses nothing in %doc for runtime.
[-]: Package is not known to require ExcludeArch.
[x]: Package complies to the Packaging Guidelines
[!]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses found:
     "Unknown or generated". 6 files have unknown license. Detailed output of
     licensecheck in /home/brain/999690-bcache-tools/licensecheck.txt
[!]: Package consistently uses macro is (instead of hard-coded directory
     names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[x]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[x]: Package contains systemd file(s) if in need.
[ ]: Useful -debuginfo package or justification otherwise.
[-]: Large documentation must go in a -doc subpackage.
     Note: Documentation size is 30720 bytes in 2 files.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Each %files section contains %defattr if rpm < 4.4
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Fully versioned dependency in subpackages, if present.
[x]: If (and only if) the source package includes the text of the license(s)
     in its own file, then that file, containing the text of the license(s)
     for the package is included in %doc.
[x]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package do not use a name that already exist
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as provided
     in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local
[x]: Package successfully compiles and builds into binary rpms on at least one
     supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).

===== SHOULD items =====

Generic:
[x]: If the source package does not include license text(s) as a separate file
     from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[x]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[x]: Patches link to upstream bugs/comments/lists or are otherwise justified.
[x]: SourceX tarball generation or download is documented.
     Note: Package contains tarball without URL, check comments
[x]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[!]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed files.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: Dist tag is present.
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define.

===== EXTRA items =====

Generic:
[x]: Large data in /usr/share should live in a noarch subpackage if package is
     arched.
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: bcache-tools-0-0.7.20130820git.fc21.x86_64.rpm
bcache-tools.x86_64: W: only-non-binary-in-usr-lib
1 packages and 0 specfiles checked; 0 errors, 1 warnings.




Rpmlint (installed packages)
----------------------------
# rpmlint bcache-tools
bcache-tools.x86_64: W: only-non-binary-in-usr-lib
1 packages and 0 specfiles checked; 0 errors, 1 warnings.
# echo 'rpmlint-done:'



Requires
--------
bcache-tools (rpmlib, GLIBC filtered):
    /bin/sh
    /usr/bin/env
    libc.so.6()(64bit)
    libuuid.so.1()(64bit)
    libuuid.so.1(UUID_1.0)(64bit)
    python
    rtld(GNU_HASH)



Provides
--------
bcache-tools:
    bcache-tools
    bcache-tools(x86-64)



Generated by fedora-review 0.4.1 (b2e211f) last change: 2013-04-29
Buildroot used: fedora-rawhide-x86_64
Command line :/usr/bin/fedora-review -m fedora-rawhide-x86_64 -b 999690

Thanks for the review!

* Sat Aug 31 2013 Rolf Fokkens <rolf> - 0-0.8.20130827git
- updated bcache-tools to commit 8327108eeaf3e0491b17d803da164c0827aae622
- corrected URL/VCS tag
- moved towards more RPM compliancy by using configure macro
- used "make install" to do most of the work
- added (empty) check section

Instead of "echo '#!/bin/sh' > ./configure" in %prep I use a %patch to create the configure file becaus the %patch will fail when at some point in the future a configures is included in the source package. 

Testing bcache tools would require root access to do serious changes on a running system, I can't imagine these tests to be good automated tests for %check.

I started a Licensing discussion upstream:
http://thread.gmane.org/gmane.linux.kernel.bcache.devel/2006


SRPM URL: http://bdsync.rolf-fokkens.nl/bcache-tools-0-0.8.20130827git/bcache-tools-0-0.8.20130827git.fc19.src.rpm
Spec URL: http://bdsync.rolf-fokkens.nl/bcache-tools-0-0.8.20130827git/bcache-tools.spec

> Instead of "echo '#!/bin/sh' > ./configure" in %prep I use a %patch

Good decision. Thumbs up for that. Less work would have been a guard like this

  [ -f configure ] && exit -1

before creating that file.

Thanks. I've updated spec w/ small fixes.
New spec: http://ignatenkobrain.fedorapeople.org/for-review/bcache-tools.spec
New SRPM: http://ignatenkobrain.fedorapeople.org/for-review/bcache-tools-0-0.9.20130827git.fc19.src.rpm

With this we could be roll, but I should to ask one question about initrd.
In upstream it create hook for initramfs. We use dracut for initrd and I don't know what do with it.
Please ask about this in devel@ mail list.

After fix this issue we can complete review.
Thanks.

(In reply to Michael Schwendt from comment #18)
> > Instead of "echo '#!/bin/sh' > ./configure" in %prep I use a %patch
> 
> Good decision. Thumbs up for that. Less work would have been a guard like
> this
> 
>   [ -f configure ] && exit -1
> 
> before creating that file.
I think we should talk about this automatically at building rpms..

Both dracut and anaconda need to be addressed for F21 I think:

http://fedoraproject.org//wiki/Changes/SSD_cache (bug 998543)

For F21 it will be SystemWide, but for now it'll be only "SelfContained". Because of this I assume that bcache-tools in its current shape suffices.

I'll start discussions both on anaconda and dracut ASAP.

Submitted patches to make blkid identify bcache superblocks, see bug 1001120

When accepted, this will impacte bcache-tools because the blkid bcache identification is more robust than the probe-bcache, so:
- probe-bcache will be obsolete
- the udev rules file provided with bcache-tools can be cleaned up

Started discussion on anaconda and dractut on devel@:
https://lists.fedoraproject.org/pipermail/devel/2013-September/188625.html

Hi all,

(In reply to Rolf Fokkens from comment #17)
> SRPM URL:
> http://bdsync.rolf-fokkens.nl/bcache-tools-0-0.8.20130827git/bcache-tools-0-
> 0.8.20130827git.fc19.src.rpm
> Spec URL:
> http://bdsync.rolf-fokkens.nl/bcache-tools-0-0.8.20130827git/bcache-tools.
> spec

As discussed with Rolf by email I will be his sponsor, so I'm now starting a full review of the above version as part of the sponsoring process.

Igor, sorry for stealing this review from you, but since you're not a sponsor you cannot approve packages from packagers which need a sponsor (iow you cannot review packages which block FE_NEEDSPONSOR)

Regards,

Hans

(In reply to Hans de Goede from comment #24)
> Hi all,
> 
> (In reply to Rolf Fokkens from comment #17)
> > SRPM URL:
> > http://bdsync.rolf-fokkens.nl/bcache-tools-0-0.8.20130827git/bcache-tools-0-
> > 0.8.20130827git.fc19.src.rpm
> > Spec URL:
> > http://bdsync.rolf-fokkens.nl/bcache-tools-0-0.8.20130827git/bcache-tools.
> > spec
> 
> As discussed with Rolf by email I will be his sponsor, so I'm now starting a
> full review of the above version as part of the sponsoring process.
ok. no problem, but use my spec and src.rpm from my comment. I removed some problems. comment 19.
> 
> Igor, sorry for stealing this review from you, but since you're not a
> sponsor you cannot approve packages from packagers which need a sponsor (iow
> you cannot review packages which block FE_NEEDSPONSOR)
> 
> Regards,
> 
> Hans
Rolf, feel free to add me as co-maintainer.

Hi,

(In reply to Igor Gnatenko from comment #25)
> ok. no problem, but use my spec and src.rpm from my comment. I removed some
> problems. comment 19.

Yes, I noticed and I've already done a diff between your spec-file and Rolf's to make sure I did not miss any of your improvements. But since I'm sponsoring Rolf, and Rolf still needs to learn all the ins and outs of packaging I have chosen to do a review of his latest version.

Regards,

Hans

Full review done:

Good:
====
- rpmlint checks return:
 bcache-tools.src: W: invalid-url Source1: bcache-status-20130826.tar.gz 
 bcache-tools.src: W: invalid-url Source0: bcache-tools-20130827.tar.bz
 bcache-tools.x86_64: W: only-non-binary-in-usr-lib
These 3 can be ignored
- package meets naming guidelines
- package meets packaging guidelines
- license (GPLv2) OK, text in %doc, matches source
- spec file legible, in am. english
- source matches upstream
- package compiles on devel (x86)
- no missing BR
- no unnecessary BR
- no locales
- not relocatable
- owns all directories that it creates
- no duplicate files
- permissions ok
- macro use consistent
- code, not content
- no need for -docs
- nothing in %doc affects runtime
- no need for .desktop file

Needs work:
========
- rpmlint checks return:
 bcache-tools.src: E: multiple-specfiles bcache-tools-20130827-fedconfmake.spec bcache-tools.spec
  One of these is a patch, it should be renamed to .patch
 bcache-tools.x86_64: E: explicit-lib-dependency libuuid
  rpmbuild automatically adds dependencies for libraries to which elf binaries link, so please drop the
  libuuid requires, but keep the python requires!
- The %patch# lines in the spec-file have trailing white-sapce, please remove this
- There is no need to use CFLAGS="%{optflags}" in front of %configure, %configure does this itself
- Please drop the empty %check section
- "make install DESTDIR=%{buildroot}" can be replaced with %make_install
- The Summary could be improved, may I suggest: "Tools for Linux kernel block layer cache"

Rolf, if you can do a new version with these fixed, I'll approve this package and sponsor you into the packagers group. If you've not done so already please create an account in the Fedora Accounts System and sign the CLA. Once you've a FAS account, please let me know your FAS username, I need that to add you to the packagers group.

About the license issues raised in comment #16, all the source code files are clearly marked as being GPLv2 (even though they don't use the standard GPL copyright header, the authors intent is clear). man-pages are often not explicitly licensed (the same goes for ie Makefiles), so I don't really see a problem here.

Thanks for your review!

I processed your feedback, the new SRPM/SPEC:

SRPM URL: http://bdsync.rolf-fokkens.nl/bcache-tools-0-0.9.20130827git/bcache-tools-0-0.9.20130827git.fc19.src.rpm
Spec URL: http://bdsync.rolf-fokkens.nl/bcache-tools-0-0.9.20130827git/bcache-tools.spec

My FAS account: rolffokkens

Igor, thanks for your offer to be a co-maintainer!

Hi,

The package looks good now, APPROVED.

Rolf, I've added you to the Packagers Group in FAS and sponsored you there so your status in it is approved. Now you can move on with step 2.1.17 of:
https://fedoraproject.org/wiki/Join_the_package_collection_maintainers?rd=PackageMaintainers/Join

Regards,

Hans

New Package SCM Request
=======================
Package Name: bcache-tools
Short Description: Tools for Linux kernel block layer cache
Owners: rolffokkens
Branches: f20
InitialCC: ignatenkobrain jreznik

Git done (by process-git-requests).

bcache-tools-0-0.9.20130827git.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/bcache-tools-0-0.9.20130827git.fc20

bcache-tools-0-0.9.20130827git.fc20 has been pushed to the Fedora 20 testing repository.

bcache-tools-0-0.10.20130827git.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/bcache-tools-0-0.10.20130827git.fc20

bcache-tools-0-0.11.20130909git.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/bcache-tools-0-0.11.20130909git.fc20

bcache-tools-0-0.11.20130909git.fc20 has been pushed to the Fedora 20 stable repository.  If problems still persist, please make note of it in this bug report.