Bug 1295217

I'm upstream maintainer. My other packages:
https://bugzilla.redhat.com/show_bug.cgi?id=1293100
https://bugzilla.redhat.com/show_bug.cgi?id=1295209

Remove the "A" article from Summary, it looks better in listings without.

There's a dot not a comma in "like JSON. but fast and small".

I think you should remove the "main" package which only contains the readme and license files, and simply include those files in both -doc and -devel. There is no guideline for this (afaik), but in general we try to avoid unnecessary subpackages.

Use %license for LICENSE [https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text].

Remove the quotes from %files.

You can also remove the quotes in %install and %build: no paths will ever contain spaces.

> I think you should remove the "main" package which only contains the readme and license files, and simply include those files in both -doc and -devel. 

How I can remove the main package? I haven't found any way except to rename source package to msgpuck-devel.

> Use %license for LICENSE [https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text].
> You can also remove the quotes in %install and %build: no paths will ever contain spaces.

OK, same problems again.

(In reply to Roman Tsisyk from comment #3)
> > I think you should remove the "main" package which only contains the readme and license files, and simply include those files in both -doc and -devel. 
> 
> How I can remove the main package? I haven't found any way except to rename
> source package to msgpuck-devel.

Simply omit the %files section.

Status update:

I've made all review fixes and I plan push the updated version of spec after finishing with the lua-fun #1295209. I need to get some experience and finish with the one package.

A new version.

Spec URL: https://gist.githubusercontent.com/rtsisyk/5bbce59a5a9954df8039/raw/2a5d454df4af66d6580bbd03c8ab931bf0b683af/msgpuck-1.0.1.spec
SRPM URL: https://gist.github.com/rtsisyk/5bbce59a5a9954df8039/raw/2a5d454df4af66d6580bbd03c8ab931bf0b683af/msgpuck-1.0.1-1.fc24.src.rpm

fedora-review output: https://gist.githubusercontent.com/rtsisyk/5bbce59a5a9954df8039/raw/2a5d454df4af66d6580bbd03c8ab931bf0b683af/review.txt

Please add 
Provides: msgpuck-static = %{version}-%{release}
to the devel subpackage [https://fedoraproject.org/wiki/Packaging:Guidelines#Packaging_Static_Libraries_2].

I don't know if you intend to provide an EPEL5 version too. If not, you can drop the license fallback: https://lists.fedoraproject.org/archives/list/devel%40lists.fedoraproject.org/message/PTZWPNI4B6Q7XF5X2TQ6GKDKYHZRIKJW/.

+ license is acceptable (BSD)
+ license file is present, %license is used
+ package name is OK
+ latest version
+ builds and installs OK
+ %check is present
+ no scriptlets required or present
+ requires and provides OK (apart from issue above)

Package is APPROVED.

>Provides: msgpuck-static = %{version}-%{release}
to the devel subpackage [https://fedoraproject.org/wiki/Packaging:Guidelines#Packaging_Static_Libraries_2].

Already:

%package devel
Summary: MsgPack serialization library in a self-contained header file
Provides: msgpuck-static = %{version}-%{release}
Requires: msgpuck%{?_isa} = %{version}-%{release}

> I don't know if you intend to provide an EPEL5 version too. If not, you can drop the license fallback

%license didn't work on CentOS 6

> Package is APPROVED.

OK, thanks!

Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/msgpuck

msgpuck-1.0.1-1.fc23 has been submitted as an update to Fedora 23. https://bodhi.fedoraproject.org/updates/FEDORA-2016-4a0bbb7dfa

msgpuck-1.0.1-1.el7 has been submitted as an update to Fedora EPEL 7. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2016-577823c5bf

msgpuck-1.0.1-1.fc22 has been submitted as an update to Fedora 22. https://bodhi.fedoraproject.org/updates/FEDORA-2016-b7b90d9f1f

msgpuck-1.0.1-1.el7 has been pushed to the Fedora EPEL 7 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2016-577823c5bf

msgpuck-1.0.1-2.fc23 has been submitted as an update to Fedora 23. https://bodhi.fedoraproject.org/updates/FEDORA-2016-ee5102307f

> buildsys
> msgpuck has broken dependencies in the rawhide tree:
> On x86_64:
>         msgpuck-devel-1.0.1-1.fc24.x86_64 requires msgpuck(x86-64) = 0:1.0.1-1.fc24
> On i386:
>         msgpuck-devel-1.0.1-1.fc24.i686 requires msgpuck(x86-32) = >0:1.0.1-1.fc24
> On armhfp:
>         msgpuck-devel-1.0.1-1.fc24.armv7hl requires msgpuck(armv7hl-32) = 0:1.0.1-1.fc24
>  Please resolve this as soon as possible.

I accidentally forgot to remove dependency on `msgpuck%{?_isa} = %{version}-%{release}` after dropping empty `msgpuck` package. Fixed. Sorry for that.

Is it possible to run the same automated tests before pushing to master?

(In reply to Roman Tsisyk from comment #16)
> Is it possible to run the same automated tests before pushing to master?

Usually, the situation with requires/provides is clearly seen even by binary RPM before submitting. E.g., "yum deplist packagename.rpm" in rhel7 shows you resolved deplist, and something like:
$ rpm -qp --requires packagename.rpm | sort | uniq -c
  and
$ rpm -qp --provides packagename.rpm | sort | uniq -c
 provides you with R/P sanity results.

As for other details:

1) Specfile has invalid name, it shouldn't contain the package version: http://pkgs.fedoraproject.org/cgit/rpms/msgpuck.git/tree/
Rpmlint check against SRPM failed;

2) If you use only rhel7 and above, you can drop "%{!?_licensedir" workaround (I don't see the el6 branch requested);

3) The recent guidelines require to specify all build requirements for new packages (in your case: make, coreutils, gcc-c++); 

4) You can save on '%cmake' options if you use the default options that are already bootstrapped in epel7 and fXX branches:
  $ rpm -E '%cmake'
  ...
  /usr/bin/cmake \
        -DCMAKE_C_FLAGS_RELEASE:STRING="-DNDEBUG" \
        -DCMAKE_CXX_FLAGS_RELEASE:STRING="-DNDEBUG" \
        -DCMAKE_Fortran_FLAGS_RELEASE:STRING="-DNDEBUG" \
        -DCMAKE_VERBOSE_MAKEFILE:BOOL=ON \
        -DCMAKE_INSTALL_PREFIX:PATH=/usr \
        -DINCLUDE_INSTALL_DIR:PATH=/usr/include \
        -DLIB_INSTALL_DIR:PATH=/usr/lib64 \
  ...
 You may use them in `msgpuck` instead of CMAKE_INSTALL_LIBDIR and CMAKE_INSTALL_INCLUDEDIR;

5) Please use empty lines between changelog entries, although there is no requirement but it helps reading changelogs.

(In reply to Denis Fateyev from comment #17)
> (In reply to Roman Tsisyk from comment #16)
> > Is it possible to run the same automated tests before pushing to master?
If you submit an update then a bunch of tests are run automatically
by Taskotron and they should catch that.

> 1) Specfile has invalid name, it shouldn't contain the package version:
> http://pkgs.fedoraproject.org/cgit/rpms/msgpuck.git/tree/
> Rpmlint check against SRPM failed;
Ooops.

> 2) If you use only rhel7 and above, you can drop "%{!?_licensedir"
> workaround (I don't see the el6 branch requested);
True.

> 3) The recent guidelines require to specify all build requirements for new
> packages (in your case: make, coreutils, gcc-c++); 

This changed recently. https://fedorahosted.org/fpc/ticket/490 and
https://fedorahosted.org/fpc/ticket/497 have the full history, but the relevant
part is the following change to guidelines
[https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRequires_2]:

"It is important that your package list all necessary build dependencies using the BuildRequires?: tag. You may assume that enough of an environment exists for RPM to function and execute basic shell scripts, but you should not assume any other packages are present as RPM dependencies and anything brought into the buildroot by the build system may change over time."

fedora-review is wrong here, and one SHOULD have BuildRequires:gcc,
though things work just fine without, and will do so for the forseeable
future. The spec file is correct.

> 4) You can save on '%cmake' options if you use the default options that are
> already bootstrapped in epel7 and fXX branches:
>   $ rpm -E '%cmake'
>   ...
>   /usr/bin/cmake \
>         -DCMAKE_C_FLAGS_RELEASE:STRING="-DNDEBUG" \
>         -DCMAKE_CXX_FLAGS_RELEASE:STRING="-DNDEBUG" \
>         -DCMAKE_Fortran_FLAGS_RELEASE:STRING="-DNDEBUG" \
>         -DCMAKE_VERBOSE_MAKEFILE:BOOL=ON \
>         -DCMAKE_INSTALL_PREFIX:PATH=/usr \
>         -DINCLUDE_INSTALL_DIR:PATH=/usr/include \
>         -DLIB_INSTALL_DIR:PATH=/usr/lib64 \
>   ...
>  You may use them in `msgpuck` instead of CMAKE_INSTALL_LIBDIR and
> CMAKE_INSTALL_INCLUDEDIR;
> 
> 5) Please use empty lines between changelog entries, although there is no
> requirement but it helps reading changelogs.

Ack. Apart from 3), those are all very valid comments.

(In reply to Zbigniew Jędrzejewski-Szmek from comment #18)
> > 3) The recent guidelines require to specify all build requirements for new
> > packages (in your case: make, coreutils, gcc-c++); 
> 
> This changed recently. https://fedorahosted.org/fpc/ticket/490 and
> https://fedorahosted.org/fpc/ticket/497 have the full history, but the
> relevant
> part is the following change to guidelines
> [https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRequires_2]:
> 
> "It is important that your package list all necessary build dependencies
> using the BuildRequires?: tag. You may assume that enough of an environment
> exists for RPM to function and execute basic shell scripts, but you should
> not assume any other packages are present as RPM dependencies and anything
> brought into the buildroot by the build system may change over time."
> 
> fedora-review is wrong here, and one SHOULD have BuildRequires:gcc,
> though things work just fine without, and will do so for the forseeable
> future. The spec file is correct.

The wording in https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRequires_2 hasn't changed since the proposed writeup https://fedorahosted.org/fpc/ticket/497 .
It's seen by current wording and the diff in the ticket above.

The exception list is gone, nobody would say what is "enough to execute basic shell scripts" on all arch now and in the future. We at 'perl-sig' are using all needed BR explicitly.

> (In reply to Roman Tsisyk from comment #16)
> > Is it possible to run the same automated tests before pushing to master?
If you submit an update then a bunch of tests are run automatically
by Taskotron and they should catch that.

> 1) Specfile has invalid name, it shouldn't contain the package version:

I renamed msgpuck.spec to msgpuck-1.0.1.spec because it was suggested by rpmlint or fedora-review :) I'll fix this problem today. Sorry.


> 2) If you use only rhel7 and above, you can drop "%{!?_licensedir" workaround (I don't see the el6 branch requested);

Yeah, I know about latest %license changes in EPEL7. Is it possible to keep this workaround? I don't want to maintain a separate version of spec for RHEL6/CentOS6.

> fedora-review is wrong here, and one SHOULD have BuildRequires:gcc,
> though things work just fine without, and will do so for the forseeable
> future. The spec file is correct.

I can add `BuildRequires: gcc`, it is not a problem. 

>> 4) You can save on '%cmake' options if you use the default options that are
>> already bootstrapped in epel7 and fXX branches:
>>   $ rpm -E '%cmake'
>>   ...
>>   /usr/bin/cmake \
>>         -DCMAKE_C_FLAGS_RELEASE:STRING="-DNDEBUG" \
>>         -DCMAKE_CXX_FLAGS_RELEASE:STRING="-DNDEBUG" \
>>         -DCMAKE_Fortran_FLAGS_RELEASE:STRING="-DNDEBUG" \
>>         -DCMAKE_VERBOSE_MAKEFILE:BOOL=ON \
>>         -DCMAKE_INSTALL_PREFIX:PATH=/usr \
>>         -DINCLUDE_INSTALL_DIR:PATH=/usr/include \
>>         -DLIB_INSTALL_DIR:PATH=/usr/lib64 \
>>   ...
>>  You may use them in `msgpuck` instead of CMAKE_INSTALL_LIBDIR and
>> CMAKE_INSTALL_INCLUDEDIR;

GNUInstallDirs.cmake from CMake doesn't work well with `%cmake` default defines. I have no idea why %cmake macro uses LIB_INSTALL_DIR instead of CMAKE_INSTALL_LIBDIR. I think either cmake package or %cmake macro should be fixed instead. I can file a ticket, if you agree with me.

(In reply to Roman Tsisyk from comment #20)
> > 2) If you use only rhel7 and above, you can drop "%{!?_licensedir" workaround (I don't see the el6 branch requested);
> 
> Yeah, I know about latest %license changes in EPEL7. Is it possible to keep
> this workaround? I don't want to maintain a separate version of spec for
> RHEL6/CentOS6.

If you're going to maintain el6, then better keep this workaround.
At least, until the situation with recently introduced `%license` support in el6 is settled.

> I can add `BuildRequires: gcc`, it is not a problem.

I'd recommend you adding all of them. It won't break things.
 
> >> 4) You can save on '%cmake' options if you use the default options that are
> >> already bootstrapped in epel7 and fXX branches:
> >>   $ rpm -E '%cmake'
> >>   ...
> >>   /usr/bin/cmake \
> >>         -DCMAKE_C_FLAGS_RELEASE:STRING="-DNDEBUG" \
> >>         -DCMAKE_CXX_FLAGS_RELEASE:STRING="-DNDEBUG" \
> >>         -DCMAKE_Fortran_FLAGS_RELEASE:STRING="-DNDEBUG" \
> >>         -DCMAKE_VERBOSE_MAKEFILE:BOOL=ON \
> >>         -DCMAKE_INSTALL_PREFIX:PATH=/usr \
> >>         -DINCLUDE_INSTALL_DIR:PATH=/usr/include \
> >>         -DLIB_INSTALL_DIR:PATH=/usr/lib64 \
> >>   ...
> >>  You may use them in `msgpuck` instead of CMAKE_INSTALL_LIBDIR and
> >> CMAKE_INSTALL_INCLUDEDIR;
> 
> GNUInstallDirs.cmake from CMake doesn't work well with `%cmake` default
> defines. I have no idea why %cmake macro uses LIB_INSTALL_DIR instead of
> CMAKE_INSTALL_LIBDIR. I think either cmake package or %cmake macro should be
> fixed instead. I can file a ticket, if you agree with me.

Well, I'm not really aware of GNUInstallDirs.cmake issue if it's exist. You can get more details from `cmake` maintainer first or asking in the list.

> If you're going to maintain el6, then better keep this workaround.
At least, until the situation with recently introduced `%license` support in el6 is settled.

I use the same spec to build package on my own buildbot. I can remove this workaround from dist git if you wish.

>  I'd recommend you adding all of them. It won't break things.

I'll add it, no problems.

>  Well, I'm not really aware of GNUInstallDirs.cmake issue if it's exist. You can get more details from `cmake` maintainer first or asking in the list.

I'll contact cmake maintainer to discuss this problem.

Thanks for your attentiveness!

(In reply to Roman Tsisyk from comment #22)
> > If you're going to maintain el6, then better keep this workaround.
> At least, until the situation with recently introduced `%license` support in
> el6 is settled.
> 
> I use the same spec to build package on my own buildbot. I can remove this
> workaround from dist git if you wish.

If you're working with el6, just keep it. I thought you were using only epel7 and above.

>  If you're working with el6, just keep it. I thought you were using only epel7 and above.

Yes, I build el6 (actually centos6) packages for a non-official repository. 
So I tried to use the same spec file.

(In reply to Denis Fateyev from comment #19)
> (In reply to Zbigniew Jędrzejewski-Szmek from comment #18)
> > > 3) The recent guidelines require to specify all build requirements for new
> > > packages (in your case: make, coreutils, gcc-c++); 
> > 
> > This changed recently. https://fedorahosted.org/fpc/ticket/490 and
> > https://fedorahosted.org/fpc/ticket/497 have the full history, but the
> > relevant
> > part is the following change to guidelines
> > [https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRequires_2]:
> > 
> > "It is important that your package list all necessary build dependencies
> > using the BuildRequires?: tag. You may assume that enough of an environment
> > exists for RPM to function and execute basic shell scripts, but you should
> > not assume any other packages are present as RPM dependencies and anything
> > brought into the buildroot by the build system may change over time."
> > 
> > fedora-review is wrong here, and one SHOULD have BuildRequires:gcc,
> > though things work just fine without, and will do so for the forseeable
> > future. The spec file is correct.
> 
> The wording in
> https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRequires_2 hasn't
> changed since the proposed writeup https://fedorahosted.org/fpc/ticket/497 .
> It's seen by current wording and the diff in the ticket above.

Sorry, I misread you comment completely. You're correct, those BR should
be added.

Spec renamed, BR fixed.
I opened #1301720 for %cmake + GNUInstallDirs.cmake problem.

msgpuck-1.0.1-2.fc23 has been pushed to the Fedora 23 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2016-ee5102307f

Looked at the current version in rawhide. Just a small nit-pick:

1) Please use "install -Dpm 0644 doc/man/man3/msgpuck.h.3* %{buildroot}%{_mandir}/man3/" instead of "cp -f" to preserve files timestamps;

2) Though I'm not really insisting, but pointing out full BRs is according the current guidelines. As pointed above, you should also add `make` and `coreutils`;

3) Changelog list will shortly become messy, e.g. during the next mass rebuild by rel-eng: they'll add a changelog entry with an empty line in the end making the whole list ragged. As also pointed above, it would better to use an empty line as the changelog items delimiter.

> 1) Please use "install -Dpm 0644 doc/man/man3/msgpuck.h.3* %{buildroot}%{_mandir}/man3/" instead of "cp -f" to preserve files timestamps;

Good catch. Thanks!

> 2) Though I'm not really insisting, but pointing out full BRs is according the current guidelines. As pointed above, you should also add `make` and `coreutils`;

I see that even core packages ignores this practice. I'm just curious do I need a dependency on kernel? :) I'll update my spec anyway, but I have no idea how to check BR on the base system. 

> 3) Changelog list will shortly become messy, e.g. during the next mass rebuild by rel-eng: they'll add a changelog entry with an empty line in the end making the whole list ragged. As also pointed above, it would better to use an empty line as the changelog items delimiter.

OK, I'll take this into account too.
Probably I need to learn some more examples from http://pkgs.fedoraproject.org/.

I'll update my spec tomorrow and push. 
I hope that is it.

(In reply to Roman Tsisyk from comment #29)
> > 2) Though I'm not really insisting, but pointing out full BRs is according the current guidelines. As pointed above, you should also add `make` and `coreutils`;
> 
> I see that even core packages ignores this practice.

It's a recent invention, so many packages just don't have these changes for historical reasons. Generally speaking, there are tons of packages in pkgdb that don't fully comply with current guidelines and will probably never be (intricate/superfluous buildroot tags, buildroot cleanup, %clean section, %license missing, etc. deprecated stuff), but new packages should certainly follow the current guidelines. 

> I'm just curious do I need a dependency on kernel? :) I'll update my spec
> anyway, but I have no idea how to check BR on the base system.

Well, it's quite simple: along build deps, you just add deps on things that are used through your package spec (make for `make`, coreutils for `mv`, `install`, `mkdir`, etc.)

msgpuck-1.0.2-1.fc23 has been submitted as an update to Fedora 23. https://bodhi.fedoraproject.org/updates/FEDORA-2016-01e318faa8

msgpuck-1.0.2-1.fc22 has been submitted as an update to Fedora 22. https://bodhi.fedoraproject.org/updates/FEDORA-2016-6a17cb3306

msgpuck-1.0.2-1.el7 has been submitted as an update to Fedora EPEL 7. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2016-6066d67c0c

msgpuck-1.0.2-1.el7 has been pushed to the Fedora EPEL 7 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2016-6066d67c0c

msgpuck-1.0.2-1.fc23 has been pushed to the Fedora 23 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2016-01e318faa8

msgpuck-1.0.2-1.fc22 has been pushed to the Fedora 22 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2016-6a17cb3306

msgpuck-1.0.2-1.el7 has been pushed to the Fedora EPEL 7 stable repository. If problems still persist, please make note of it in this bug report.

msgpuck-1.0.2-1.fc22 has been pushed to the Fedora 22 stable repository. If problems still persist, please make note of it in this bug report.

msgpuck-1.0.2-1.fc23 has been pushed to the Fedora 23 stable repository. If problems still persist, please make note of it in this bug report.