Bug 1476458

>Group: Development/Tools
>Group: Development/Libraries
The Group: tag should not be used.
https://fedoraproject.org/wiki/Packaging:Guidelines#Tags_and_Sections

>%package devel
>Requires: paho-c
This allows to use the -devel package with any version of the main package. The versions should be an exact match. You can use "Requires: %{name}%{?_isa} = %{version}-%{release}" to achieve this.

>%doc edl-v10 epl-v10
These files should be marked as %license.
https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text

I didn't try building the package, but as far as I can see, the devel-docs package only contains Doxygen-generated documentation. You can mark the package as "BuildArch: noarch" to make it an architecture-agnostic package.

Thanks for your review Iwicki. 

I have modified all the items you have pointed. Here's some updated links: 

Latest COPR build for reference: https://copr.fedorainfracloud.org/coprs/orpiske/paho-testing/build/584853/
Updated successful Koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=20899923
Upstream discussion: https://github.com/eclipse/paho.mqtt.c/issues/315 (we switched to an issue instead of a PR discussion upstream)

The link to the spec remains the same: https://github.com/orpiske/paho.mqtt.c/blob/fedora-upstream-changes/dist/paho-c.spec

Finally, I would kindly ask for an advice regarding the dependencies for the docs package: should I enforce the same "Requires: %{name}%{?_isa} = %{version}-%{release}" on the docs package as well?

In general, it's best if the docs can be installed without having to pull in any parts of their parent package.
>Files marked as documentation must not cause the package to pull in more dependencies than it would without the documentation.
https://fedoraproject.org/wiki/Packaging:Guidelines#Documentation

As a general rule, %{_isa} and "BuildArch: noarch" do not go along. %{_isa} expands to the architecture description, so it creates an arch-specific dependency. 

Also, last thing - as mentioned in the link above, the subpackage name should be -doc, not -docs (without the "s").

Thanks for the explanation. 

I renamed the package to -doc. 

Here's the latest Koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=20900568. The link to the spec remains the same.

Sorry for not pointing this out earlier, but I just consulted the Licensing Guidelines:
https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Valid_License_Short_Names

>The License: field must be filled with the appropriate license Short License identifier(s) from the "Good License" tables on the Fedora Licensing page.
"Eclipse Distribution License 1.0" shortname is "BSD", while "Eclipse Public License 1.0" shortname is "EPL". This means that instead of
>License: Eclipse Distribution License 1.0 and Eclipse Public License 1.0
you should use
>License: BSD and EPL

> Sorry for not pointing this out earlier, but I just consulted the Licensing Guidelines:
https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Valid_License_Short_Names

Thanks for pointing this out. I adjusted the License text as explained.

Latest Koji build is available here: https://koji.fedoraproject.org/koji/taskinfo?taskID=20925743
Copr build: https://copr.fedorainfracloud.org/coprs/orpiske/paho-testing/build/585162/

Any other recommendation about how I could improve this spec and have it comply with Fedora packaging guidelines?

Hello,

A couple of points:

 - make %{?_smp_mflags} could be replaced with %make_build and make install DESTDIR=%{buildroot} with %make_install
 - I believe you should include %license in the devel-docs package as it can be installed independently of others.

> Requires: openssl

https://fedoraproject.org/wiki/Packaging:Guidelines#Explicit_Requires


> %files
> %{_libdir}/*

Just from the looks of it and the -devel package, this is likely wrong.
See:
https://fedoraproject.org/wiki/Packaging:Guidelines#Shared_Libraries
https://fedoraproject.org/wiki/Packaging:Guidelines#Devel_Packages


> %files devel-doc
> %{_datadir}/*

https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Subpackage_Licensing

Thank you for your review! 

I applied the changes you suggested. The link to the spec is: https://github.com/orpiske/paho.mqtt.c/blob/fedora-upstream-changes/dist/paho-c.spec

The successful Koji build is available here: https://koji.fedoraproject.org/koji/taskinfo?taskID=21179367

I am not entirely sure that the way I did the split between the actual shared libraries the symlinks are correct. I used %{_libdir}/*.so for the symlinks and %{_libdir}/*.so.* for the actual libraries. Although this does seem to do the split correctly:

rpm -qf /usr/lib64/libpaho-mqtt3*.so
paho-c-devel-1.2.0-8.fc26.x86_64
paho-c-devel-1.2.0-8.fc26.x86_64
paho-c-devel-1.2.0-8.fc26.x86_64
paho-c-devel-1.2.0-8.fc26.x86_64

rpm -qf /usr/lib64/libpaho-mqtt3*.so.*
paho-c-1.2.0-8.fc26.x86_64
paho-c-1.2.0-8.fc26.x86_64
paho-c-1.2.0-8.fc26.x86_64
paho-c-1.2.0-8.fc26.x86_64
paho-c-1.2.0-8.fc26.x86_64
paho-c-1.2.0-8.fc26.x86_64
paho-c-1.2.0-8.fc26.x86_64
paho-c-1.2.0-8.fc26.x86_64

I wasn't sure about the correct way to split, but I will change it if it's wrong. 

Thanks!

> I used %{_libdir}/*.so for the symlinks and %{_libdir}/*.so.* for the
> actual libraries. 

That's correct, and if you think about it a bit, it has to be like that, if splitting the files into different subpackages. Or else the main package would contain dangling symlinks.

Thanks for the explanation Michael.

Dear Fedora community, any additional suggestion about how I could improve this package?

Michael,

I've talked to Otavio and I'd like to step in and have this new package under my user (and I'm already a fedora packager). Considering this case, would be possible to get a fedora-review + from you?

I am a sponsor and I can finish this review.

(In reply to Miroslav Suchý from comment #14)
> I am a sponsor and I can finish this review.

Okay, then we can continue the process with the package under Otavio.

Thanks for the help, Miroslav.

Can you please provide updated SRC.RPM? The latest I was able to find
  https://copr-be.cloud.fedoraproject.org/results/orpiske/paho-testing/fedora-26-x86_64/00589833-paho-c/
contains different tar.gz file from the one located on the Source0 URL.
paho-c.src: W: file-size-mismatch v1.2.0.tar.gz = 441242, https://github.com/eclipse/paho.mqtt.c/archive/v1.2.0.tar.gz = 431819


The description should be wrapped to 80 characters (you have 81).

paho-c.x86_64: W: shared-lib-calls-exit /usr/lib64/libpaho-mqtt3a.so.1.2.0 exit.5
paho-c.x86_64: W: shared-lib-calls-exit /usr/lib64/libpaho-mqtt3as.so.1.2.0 exit.5
paho-c.x86_64: W: shared-lib-calls-exit /usr/lib64/libpaho-mqtt3c.so.1.2.0 exit.5
paho-c.x86_64: W: shared-lib-calls-exit /usr/lib64/libpaho-mqtt3cs.so.1.2.0 exit.5
shared-lib-calls-exit:
This library package calls exit() or _exit(), probably in a non-fork()
context. Doing so from a library is strongly discouraged - when a library
function calls exit(), it prevents the calling program from handling the
error, reporting it to the user, closing files properly, and cleaning up any
state that the program has. It is preferred for the library to return an
actual error code and let the calling program decide how to handle the
situation.

You cannot do anything about it as just maintainer. But you should at least file issue for upstream.

paho-c.x86_64: W: crypto-policy-non-compliance-openssl /usr/lib64/libpaho-mqtt3as.so.1.2.0 SSL_CTX_set_cipher_list
paho-c.x86_64: W: crypto-policy-non-compliance-openssl /usr/lib64/libpaho-mqtt3cs.so.1.2.0 SSL_CTX_set_cipher_list
Again. File issue to upstream:
https://fedoraproject.org/wiki/Packaging:CryptoPolicies

paho-c-devel.x86_64: W: no-manual-page-for-binary MQTTAsync_publish
paho-c-devel.x86_64: W: no-manual-page-for-binary MQTTAsync_subscribe 
paho-c-devel.x86_64: W: no-manual-page-for-binary MQTTClient_publish
paho-c-devel.x86_64: W: no-manual-page-for-binary MQTTClient_publish_async
paho-c-devel.x86_64: W: no-manual-page-for-binary MQTTClient_subscribe
paho-c-devel.x86_64: W: no-manual-page-for-binary MQTTVersion
paho-c-devel.x86_64: W: no-manual-page-for-binary paho_c_pub
paho-c-devel.x86_64: W: no-manual-page-for-binary paho_c_sub
paho-c-devel.x86_64: W: no-manual-page-for-binary paho_cs_pub
paho-c-devel.x86_64: W: no-manual-page-for-binary paho_cs_sub
This is not blocker for the review, but you should write man pages for those binaries.

paho-c-devel-doc.noarch: E: standard-dir-owned-by-package /usr/share/doc
You should not own that directory. Instead of
  %{_datadir}/*
use
  %{_defaultdocdir}/*

Are you sure that main package should not contain any /usr/bin/SOMETHING? It now contains only library. Then such package should be named libpaho-c.

I would highly recommend renaming `%package devel-doc` to just `%package doc`. The doc subpackage for such packages is nearly always meant for developers.

Thanks for the review Miroslav. 

Just to provide an update to those following this ticket. I have already started to work on Miroslav' comments and I hope to provide an updated package soon.

(In reply to Miroslav Suchý from comment #16)
> Can you please provide updated SRC.RPM? The latest I was able to find
>  
> https://copr-be.cloud.fedoraproject.org/results/orpiske/paho-testing/fedora-
> 26-x86_64/00589833-paho-c/
> contains different tar.gz file from the one located on the Source0 URL.
> paho-c.src: W: file-size-mismatch v1.2.0.tar.gz = 441242,
> https://github.com/eclipse/paho.mqtt.c/archive/v1.2.0.tar.gz = 431819

Done. It looks like the package was generated incorrectly because my COPR was configured to build from my repository instead of the upstream. I reconfigured it and it should be fine now. 

The link to the latest SRPM is: https://copr-be.cloud.fedoraproject.org/results/orpiske/paho-testing/fedora-26-x86_64/00654734-paho-c/paho-c-1.2.0-10.fc26.src.rpm (other builds are available at https://copr.fedorainfracloud.org/coprs/orpiske/paho-testing/build/654734/)


> 
> 
> The description should be wrapped to 80 characters (you have 81).

Fixed as requested.

> 
> paho-c.x86_64: W: shared-lib-calls-exit /usr/lib64/libpaho-mqtt3a.so.1.2.0
> exit.5
> paho-c.x86_64: W: shared-lib-calls-exit /usr/lib64/libpaho-mqtt3as.so.1.2.0
> exit.5
> paho-c.x86_64: W: shared-lib-calls-exit /usr/lib64/libpaho-mqtt3c.so.1.2.0
> exit.5
> paho-c.x86_64: W: shared-lib-calls-exit /usr/lib64/libpaho-mqtt3cs.so.1.2.0
> exit.5
> shared-lib-calls-exit:
> This library package calls exit() or _exit(), probably in a non-fork()
> context. Doing so from a library is strongly discouraged - when a library
> function calls exit(), it prevents the calling program from handling the
> error, reporting it to the user, closing files properly, and cleaning up any
> state that the program has. It is preferred for the library to return an
> actual error code and let the calling program decide how to handle the
> situation.
> 
> You cannot do anything about it as just maintainer. But you should at least
> file issue for upstream.

Noted. I opened an issue upstream: https://github.com/eclipse/paho.mqtt.c/issues/342. I'll propose a patch for this one in one of the upcoming versions.

> 
> paho-c.x86_64: W: crypto-policy-non-compliance-openssl
> /usr/lib64/libpaho-mqtt3as.so.1.2.0 SSL_CTX_set_cipher_list
> paho-c.x86_64: W: crypto-policy-non-compliance-openssl
> /usr/lib64/libpaho-mqtt3cs.so.1.2.0 SSL_CTX_set_cipher_list
> Again. File issue to upstream:
> https://fedoraproject.org/wiki/Packaging:CryptoPolicies

I opened an issue upstream (though acceptance of this one might be subject to other factors which I may not be fully aware of). 

https://github.com/eclipse/paho.mqtt.c/issues/347

> 
> paho-c-devel.x86_64: W: no-manual-page-for-binary MQTTAsync_publish
> paho-c-devel.x86_64: W: no-manual-page-for-binary MQTTAsync_subscribe 
> paho-c-devel.x86_64: W: no-manual-page-for-binary MQTTClient_publish
> paho-c-devel.x86_64: W: no-manual-page-for-binary MQTTClient_publish_async
> paho-c-devel.x86_64: W: no-manual-page-for-binary MQTTClient_subscribe
> paho-c-devel.x86_64: W: no-manual-page-for-binary MQTTVersion
> paho-c-devel.x86_64: W: no-manual-page-for-binary paho_c_pub
> paho-c-devel.x86_64: W: no-manual-page-for-binary paho_c_sub
> paho-c-devel.x86_64: W: no-manual-page-for-binary paho_cs_pub
> paho-c-devel.x86_64: W: no-manual-page-for-binary paho_cs_sub
> This is not blocker for the review, but you should write man pages for those
> binaries.

There are some changes to be done on the code (as well as documenting these). I am looking forward to provide the man pages for these in the next version. In the mean time, I opened a ticket to track that: https://github.com/eclipse/paho.mqtt.c/issues/346

> 
> paho-c-devel-doc.noarch: E: standard-dir-owned-by-package /usr/share/doc
> You should not own that directory. Instead of
>   %{_datadir}/*
> use
>   %{_defaultdocdir}/*

Noted.

> 
> Are you sure that main package should not contain any /usr/bin/SOMETHING? It
> now contains only library. Then such package should be named libpaho-c.

I had to review this within the code. I reorganized the binaries in a way that (IMHO) are more adequate now. The 4 paho_c* programs can be used as tools to publish and/or subscribe from topics via command-line, whereas the /usr/bin/MQTT* are the compiled samples that demonstrate basic Paho MQTT C coding concepts. 

I am not entirely about the validity of providing compiled binaries for the samples. Any thoughts on that?

> 
> I would highly recommend renaming `%package devel-doc` to just `%package
> doc`. The doc subpackage for such packages is nearly always meant for
> developers.

Noted. Based on your suggestion, I renamed it.

One last comment is regarding the fedpkg build: I will provide the link to the latest one as soon as I finish setting up my workstation.

Adding the missing stuff from my previous comment:

1. Link to the Koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=22721567

2. Updated spec file used for the build: https://raw.githubusercontent.com/orpiske/paho.mqtt.c/eec5bad3ea79d753e4f315d7919ee490c5361b45/dist/paho-c.spec

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

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



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

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Header files in -devel subpackage, if present.
[x]: ldconfig called in %post and %postun if required.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.
[x]: Development (unversioned) .so files in -devel subpackage, if present.

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: "BSD (3 clause)", "GPL (v2)", "*No copyright* EPL (v1.0)",
     "Unknown or generated", "EPL (v1.0)". 106 files have unknown license.
     Detailed output of licensecheck in /tmp/paho-c/licensecheck.txt
[x]: License file installed when any subpackage combination is installed.
[!]: If the package is under multiple licenses, the licensing breakdown
     must be documented in the spec.
[x]: %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.
[x]: 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.
[-]: Package is not known to require an ExcludeArch tag.
[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]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 0 bytes in 0 files.
[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).
[x]: Fully versioned dependency in subpackages if applicable.
     Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in
     paho-c-doc , paho-c-debuginfo
[?]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[x]: Scriptlets must be sane, if used.
[-]: 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.


So only remaining issue is:
[!]: If the package is under multiple licenses, the licensing breakdown
     must be documented in the spec.
Please put the comment just before "License: " tag in preamble. And in that comment explain which part is under  BSD and which under EPL
When I am checking the code, some have EPL header, but I am not sure which part is licensed under BSD. You may check that with upstream.

Great news! 

Before I check with upstream, I would like to comment that the BSD in the license comes from the feedback received in the comment #5. 

In that comment, it is stated the short form for the "Eclipse Distribution License 1.0" is BSD (as confirmed by the wiki page at https://fedoraproject.org/wiki/Licensing:Main?rd=Licensing). 

Both the licenses (Eclipse Distribution License 1.0 as well as Eclipse Public License 1.0) cover the distribution of the package (as both of the licenses appear in the header comments of the source code as well as provided along with the code). 

My understanding is that both licenses cover the package as whole. 

Based on that, could you, please, confirm that it would be sufficient to confirm that with upstream and, as a result, annex a comment stating that confirmation on top of the license tag?

Ah, right. Then there is no need to alter the spec. Sorry for the noise.

APPROVED

(fedrepo-req-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/paho-c

paho-c-1.2.0-10.fc26 has been submitted as an update to Fedora 26. https://bodhi.fedoraproject.org/updates/FEDORA-2017-405eddd75b

paho-c-1.2.0-10.fc27 has been submitted as an update to Fedora 27. https://bodhi.fedoraproject.org/updates/FEDORA-2017-38d23add48

paho-c-1.2.0-10.fc27 has been pushed to the Fedora 27 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-2017-38d23add48

paho-c-1.2.0-10.fc26 has been pushed to the Fedora 26 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-2017-405eddd75b

paho-c-1.2.0-10.fc27 has been pushed to the Fedora 27 stable repository. If problems still persist, please make note of it in this bug report.

paho-c-1.2.0-10.fc26 has been pushed to the Fedora 26 stable repository. If problems still persist, please make note of it in this bug report.

paho-c-1.2.0-10.el6 has been submitted as an update to Fedora EPEL 6. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2018-f9bab306e7

paho-c-1.2.0-10.el7 has been submitted as an update to Fedora EPEL 7. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2018-c6252bfaca

paho-c-1.2.0-10.el6 has been pushed to the Fedora EPEL 6 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-2018-f9bab306e7

paho-c-1.2.0-10.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-2018-c6252bfaca

paho-c-1.2.0-10.el7 has been pushed to the Fedora EPEL 7 stable repository. If problems still persist, please make note of it in this bug report.

paho-c-1.2.0-10.el6 has been pushed to the Fedora EPEL 6 stable repository. If problems still persist, please make note of it in this bug report.