Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 1338537 - Review Request: tarmux - Multiplex and demultiplex streams using tar file fragments.
Summary: Review Request: tarmux - Multiplex and demultiplex streams using tar file fra...
Keywords:
Status: ASSIGNED
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Ralf Senderek
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: FE-NEEDSPONSOR
TreeView+ depends on / blocked
 
Reported: 2016-05-22 14:40 UTC by Graham Leggett
Modified: 2020-04-30 15:02 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed:
Type: ---
Embargoed:
fedora: fedora-review+


Attachments (Terms of Use)

Description Graham Leggett 2016-05-22 14:40:22 UTC
Spec URL: https://github.com/minfrin/tarmux/blob/master/tarmux.spec.in
SRPM URL: https://github.com/minfrin/tarmux/releases/download/tarmux-1.0.0/tarmux-1.0.0-1.src.rpm
Description: Multiplex and demultiplex streams using tar file fragments.
Fedora Account System Username: minfrin

Comment 3 Neal Gompa 2016-05-22 15:24:48 UTC
Even before initiating a Fedora review, there are several problems with this spec.

Aside from the fact that you published a spec.in file (which is not acceptable, you *must* publish the spec that is actually used in the source RPM), the packaging style is completely wrong.

Here are the issues:

* Defined BuildRoot and Packager. While BuildRoot doesn't hurt anything and is usually ignored, Packager is *never* supposed to be defined. That *must* be stripped out. %buildroot is automatically defined by RPM during build time.

* Using overloaded macro variables to define fields. While this is not exactly not permitted, overloading the macro variables to define the fields is not. Please just put them into the fields themselves. It's redundant and pointless since the fields define those macro variables anyway.

* Not using a path to sources. It makes life easier if the Source field is defined with a direct path to it. For example: "https://github.com/minfrin/%{name}/releases/download/%{name}-%{version}/%{name}-%{version}.tar.bz2" instead of just %{name}-%{version}.tar.bz2.

* No usage of %{dist} in Release. Your release should be X%{?dist} instead of just X. For example, your Release field should be set to "1%{?dist}". See https://fedoraproject.org/wiki/Packaging:DistTag

* Using Prefix. This is not permitted in Fedora at all. Please do not attempt to make a relocatable package. It causes too many weird things to happen.

* Inadequate specification of build-time dependencies. Please specify all the components required to build (including compiler, etc.). We do not guarantee that a compiler will be present in the buildroot (even though currently one is always present).

* Not using parallel make. Please use %make_build to specify parallel make build after running %configure. By default, Fedora prefers builds to be run with make -jX, and %make_build properly handles that for you. Run "rpm -E '%make_build'" to verify for yourself.

* Redundant specification of parameters. The %configure macro already specifies all the things you're laying out there. Run "rpm -E '%configure'" to verify for yourself.

* Empty scriptlet sections (%pre, %post, %preun). Please delete these as they add unnecessary bloat to the generated RPMs.

* Redundant %clean section: Unless you're packaging for RHEL/CentOS 5, you don't need it, as RPM already has it pre-defined.

* Redundant buildroot cleaning in %install phase and non-usage of %make_install macro. Again, unless you're targeting RHEL/CentOS 5, you don't need to clean the buildroot at the beginning of the %install section. As for the install, please use '%make_install'. It does the same thing, except it's cleaner. Run "rpm -E '%make_install'" to verify for yourself.

* Usage of %attr in the file list. Do you really need this? Wouldn't the installation from your build scripts already set this? If not, perhaps you should fix it there. Usage of %attr is usually unnecessary if the build system can set it when make install is run.

Please address all of these.

Comment 4 Graham Leggett 2016-05-22 15:28:35 UTC
Do you have an example of a "hello world" spec file with all of the above in place already?

The spec file is very old and has been "handed down" from many projects. It would be far easier if there was a canonical spec file to use a base template to start again.

Comment 5 Neal Gompa 2016-05-22 16:03:32 UTC
There's an example on the Fedora Developer site: https://developer.fedoraproject.org/deployment/rpm/about.html

Comment 6 Graham Leggett 2016-05-22 17:16:28 UTC
I went through the spec file and made the changes you specified.

Try this:

Spec URL: https://github.com/minfrin/tarmux/blob/master/tarmux.spec

> Inadequate specification of build-time dependencies. Please specify
> all the components required to build (including compiler, etc.)

Can you provide an example of how to do this? I've never encountered such a requirement before and am struggling to find examples, probably because the packages I've been building are based on other devel packages that provide the needed packages.

Comment 7 Ralf Senderek 2016-05-22 17:34:16 UTC
Your new spec file looks much better now.

It's always a good idea to run rpmlint against your newly built rpm files.
Its output is not always correct, but it gives a hint where issues may turn
up in the formal review.

One point that is checked thoroughly in the review is the license of your files.
You are using a standard license which has a short name of "ASL 2.0" according to

https://fedoraproject.org/wiki/Licensing:Main?rd=Licensing#SoftwareLicenses

So if you delete the "v" you should be good here.

Your source file seems to be taken from a debian background, it contains many files that make sense only if you build a deb package. If possible, I recommend that you include only the few necessary files in the source code archive, because otherwise you'll end up with a number of files for which the review tool cannot assign a proper license.

If you wish to see the results of an automatic package review for your package, 
install fedora-review and run "fedora-review -n tarmux" as a normal user in 
a directory where your spec file and SRPM exist. (You may have to add that user to the mock group.)

Comment 8 Ralf Senderek 2016-05-22 17:42:11 UTC
(In reply to Graham Leggett from comment #6)

> > Inadequate specification of build-time dependencies. Please specify
> > all the components required to build (including compiler, etc.)
> 
> Can you provide an example of how to do this? 

You can specify a number of BuildRequires. You'd probably need  BuildRequires=gcc
anyway. The automatic review tool will attempt to do a mock-build, which creates a minimal build environment based on your specification in the spec file. If the mock-build fails the build log shows you which requirements might not be satisfied. It won't actually complain about gcc, but if other libraries were needed to build you'd see it there.

Comment 9 Graham Leggett 2016-05-22 18:02:58 UTC
Just updated it to remove the v, and fedora-review seems happy.

One of the details fedora-review complains about is that it cannot download the source, claiming 403 Forbidden. Works when I download it in a browser, seems to be github weirdness.

tarmux.src: W: invalid-url Source0: https://github.com/minfrin/tarmux/releases/d
ownload/tarmux-1.0.0/tarmux-1.0.0.tar.bz2 HTTP Error 403: Forbidden

The debian files are for packaging for private debian repositories, and these files are ignored for the RPM packaging. The package will eventually be submitted to Debian, but Fedora first.

Comment 10 Ralf Senderek 2016-05-22 18:59:48 UTC
(In reply to Graham Leggett from comment #9)

> The debian files are for packaging for private debian repositories, and
> these files are ignored for the RPM packaging. The package will eventually
> be submitted to Debian, but Fedora first.

But even debian don't expect these debian-specific files to be in the source tarball, as they are added during the debian packaging process.

Comment 11 Graham Leggett 2016-05-22 19:25:21 UTC
I suspect you're not familiar with the use of private yum/apt-get repositories for distributing software in the corporate world. The distros are not the only sources of packaged software, there is extensive use of private repos, and they need to keep their metadata somewhere. RPM is easy, a single spec file in the root of the package and all is done. Debian, being older, is more tricky, but the same pattern works. Bottom line is if you're a distro ignore the internal packaging and use your own.

Comment 12 Ralf Senderek 2016-05-23 07:49:55 UTC
Just a few small changes before I start the formal review:

1) I understand that you intend to maintain a single source tarball that can be used to package in different distributions.

At the moment this leads to 14 files with no known licenses.
It is quite easy to resolve this as you can remove files from the builddirectory
in the %prep macro before the software is compiled.

Just change:

%prep
%setup -q
rm -rf %{_builddir}/%{name}-%{version}/debian
rm -rf %{_builddir}/%{name}-%{version}/any-other-unused-file

2) Please remove NEWS, because it is an empty file.

3) If you have officially build release 1, change the release to 2 and
include the new spec file in a new SRPM. This is important, because at the moment your SRPM contains the old initial spec file. 

I recommend to keep both the new spec file and the SRPM up-to-date whenever you make any changes. And please update the changelog in the spec-file.

Once  this is done, I'll start the review.

Comment 13 Graham Leggett 2016-05-23 22:37:48 UTC
I created a new release to cover the changes, as they have been significant.

Spec URL: https://github.com/minfrin/tarmux/blob/master/tarmux.spec
SRPM URL: https://github.com/minfrin/tarmux/releases/download/tarmux-1.0.1/tarmux-1.0.1-1.fc23.src.rpm

The changes have been made above, can you verify for me?

Comment 14 Ralf Senderek 2016-05-24 13:12:32 UTC
Package Review
==============

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


Issues:
=======
- All build dependencies are listed in BuildRequires, except for any that
  are listed in the exceptions section of Packaging Guidelines.
  Note: These BR are not needed: gcc
  See: http://fedoraproject.org/wiki/Packaging/Guidelines#Exceptions_2

You can safely ignore this issue. It doesn't hurt to mention gcc.

- Some files in the source tarball are reported as having no license:
  (see below)

tarmux-1.0.1/AUTHORS
tarmux-1.0.1/COPYING
tarmux-1.0.1/ChangeLog
tarmux-1.0.1/INSTALL
tarmux-1.0.1/NEWS
tarmux-1.0.1/README
tarmux-1.0.1/compile
tarmux-1.0.1/configure
tarmux-1.0.1/depcomp
tarmux-1.0.1/missing

Most of them are clearly data files and don't need a license, but compile,
configure, depcomp and missing are shell scripts (code), that need one. 
At a closer look, it turns out that all have a valid GPL license. 
So practically there is no need to remove any of them.

If you wish to avoid their presence though, you can remove any of these files
in %prep.
This might be more important, if you have a more complex package than this one.

So any changes here are cosmetic and do not block your package.


===== 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.
[x]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses
     found: "MIT/X11 (BSD like)", "Apache (v2.0)", "Unknown or generated".
     10 files have unknown license. Detailed output of licensecheck in
     /x/fedora/reviews/review-tarmux/licensecheck.txt

see above: this is not a blocker.

[-]: License file installed when any subpackage combination is installed.
[x]: %build honors applicable compiler flags or justifies otherwise.
[-]: 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 uses nothing in %doc for runtime.
[x]: Package consistently uses macros (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.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: Useful -debuginfo package or justification otherwise.
[x]: Package is not known to require an ExcludeArch tag.
[-]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 10240 bytes in 3 files.
[x]: Package complies to the Packaging Guidelines
[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).
[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 %license.
[x]: Package requires other packages for directories it uses.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Dist tag is present.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package use %makeinstall only when make install DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package does not use a name that already exists.
[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

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

Generic:
[-]: 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).
[!]: Fully versioned dependency in subpackages if applicable.
     Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in tarmux-
     debuginfo
[x]: Package functions as described.
     Allthough the file permissions of tar file content are unusually open (666)
[?]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[-]: 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]: 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]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

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

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


Rpmlint
-------
Checking: tarmux-1.0.1-1.fc23.x86_64.rpm
          tarmux-debuginfo-1.0.1-1.fc23.x86_64.rpm
          tarmux-1.0.1-1.fc23.src.rpm
tarmux.src: W: spelling-error Summary(en_US) demultiplex -> multiplex, multiple
tarmux.src: W: spelling-error %description -l en_US demultiplex -> multiplex, multiple
tarmux.src:10: W: mixed-use-of-spaces-and-tabs (spaces: line 3, tab: line 10)
tarmux.src: W: invalid-url Source0: https://github.com/minfrin/tarmux/releases/download/tarmux-1.0.1/tarmux-1.0.1.tar.bz2 HTTP Error 403: Forbidden

This is not true, as wget works fine on this url.

tarmux.x86_64: W: spelling-error Summary(en_US) demultiplex -> multiplex, multiple
tarmux.x86_64: W: spelling-error %description -l en_US demultiplex -> multiplex, multiple
2 packages and 0 specfiles checked; 0 errors, 6 warnings.


Rpmlint (debuginfo)
-------------------
Checking: tarmux-debuginfo-1.0.1-1.fc23.x86_64.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.


Rpmlint (installed packages)
----------------------------
sh: /usr/bin/python: No such file or directory
tarmux-debuginfo.x86_64: E: no-changelogname-tag
tarmux-debuginfo.x86_64: W: invalid-license ASL v2.0
tarmux.x86_64: E: no-changelogname-tag
tarmux.x86_64: W: invalid-license ASL v2.0
tarmux.x86_64: W: empty-%pre
tarmux.x86_64: W: empty-%post
tarmux.x86_64: W: empty-%preun
2 packages and 0 specfiles checked; 2 errors, 5 warnings.

This output reflects the old specfile! These errors are addressed in the new one.


Requires
--------
tarmux-debuginfo (rpmlib, GLIBC filtered):

tarmux (rpmlib, GLIBC filtered):
    libarchive.so.13()(64bit)
    libc.so.6()(64bit)
    rtld(GNU_HASH)



Provides
--------
tarmux-debuginfo:
    tarmux-debuginfo
    tarmux-debuginfo(x86-64)

tarmux:
    tarmux
    tarmux(x86-64)



Source checksums
----------------
https://github.com/minfrin/tarmux/releases/download/tarmux-1.0.1/tarmux-1.0.1.tar.bz2 :
  CHECKSUM(SHA256) this package     : 469a38e845d456ce75a99e55b7fb141a9e269faf0d37ddb26f8f5856bbd6e0d9
  CHECKSUM(SHA256) upstream package : 469a38e845d456ce75a99e55b7fb141a9e269faf0d37ddb26f8f5856bbd6e0d9


Generated by fedora-review 0.6.1 (f03e4e7) last change: 2016-05-02
Command line :/usr/bin/fedora-review -n tarmux
Buildroot used: fedora-23-x86_64
Active plugins: Generic, Shell-api, C/C++
Disabled plugins: Java, Python, fonts, SugarActivity, Ocaml, Perl, Haskell, R, PHP
Disabled flags: EXARCH, DISTTAG, EPEL5, BATCH, EPEL6

Comment 15 Ralf Senderek 2016-05-24 13:27:26 UTC
As none of the issues mentioned in the package review need addressing, your package "tarmux" is  ***APPROVED***

Feel free to adapt any of my suggestions into your future spec file for that package.

Unfortunately I'm not a sponsor, so you'll need to look for a person that is willing to sponsor you into the packager group. So you will need to submit another more complex package for review and you will need to show your understanding of the packaging guidelines to your sponsor by commenting on other people's review requests. As long as you have not been sponsored, you can comment on other package reviews (and you should do so once you are familiar with the guidelines) but you cannot perform a complete package review.

I hope you'll find a sponsor soon, have a look at this group, feel free to contact anyone you know directly. 

https://admin.fedoraproject.org/accounts/group/members/packager/*/sponsor?_csrf_token=fa74806a4aef0d50d44894f07eab2f0e2340054f

and 

https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group

In order to attract attention, you should set a flag in one of your new package reviews FE-NEEDSPONSOR. Just enter this in the Blocks: field when you create a new bug for a review.

All the best.

Ralf

Comment 16 Graham Leggett 2016-05-31 00:34:26 UTC
First package, so requesting a sponsor.

Comment 17 Mukundan Ragavan 2016-06-03 01:20:42 UTC
(In reply to Graham Leggett from comment #16)
> First package, so requesting a sponsor.

Are you still in need of a sponsor?

Comment 18 Graham Leggett 2016-06-09 13:20:00 UTC
I am, yes.


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