https://bugzilla.redhat.com/show_bug.cgi?id=1964682

Joshua Clayton stillcompiling@gmail.com changed:

What |Removed |Added ---------------------------------------------------------------------------- CC| |stillcompiling@gmail.com Flags| |needinfo? | |needinfo?(package-review@li | |sts.fedoraproject.org)

https://bugzilla.redhat.com/show_bug.cgi?id=1964682

Robert-André Mauchin 🐧 zebob.m@gmail.com changed:

What |Removed |Added ---------------------------------------------------------------------------- CC| |zebob.m@gmail.com Doc Type|--- |If docs needed, set a value Flags|needinfo? | |needinfo?(package-review@li | |sts.fedoraproject.org) |

--- Comment #1 from Robert-André Mauchin 🐧 zebob.m@gmail.com --- - Group: Development/Tools

Group is not used in Fedora

- License: Eclipse Distribution License 1.0 and Eclipse Public License 1.0

This is not valid, we use shorthand for the licenses, check the valid ones at https://fedoraproject.org/wiki/Licensing:Main?rd=Licensing#SoftwareLicenses

License: BSD and EPL-1.0

- Requires: openssl Requires: paho-c >= 1.3.1

These should be autodetected.

- Use a more explicit name for your archive:

Source: https://github.com/eclipse/paho.mqtt.cpp/archive/v%%7Bversion%7D/%%7Bname%7D...

- Latest version is 1.2.0

- Please add a comment justifying why that patch is needed:

Patch0: paho1.1_logremove.patch

It does not seem necessary anymore with 1.2.0

- Licenses must be installed with %license not %doc:

%license edl-v10 epl-v10

- Requires: paho-cpp

Almost ok but read this https://docs.fedoraproject.org/en-US/packaging-guidelines/#_requiring_base_p... Thus is should be:

Requires: %{name}%{?_isa} = %{version}-%{release}

- You create a devel-docs package but do not assign any files to it in %files section. There should be a:

%files devel-docs %license edl-v10 epl-v10 %doc %{_docdir}/%{name}/samples/ %doc %{_docdir}/%{name}/html/

- Not needed:

mkdir build.paho.cpp && cd build.paho.cpp

The %cmake macro already does something similar. So use:

%build %cmake -DPAHO_WITH_SSL=TRUE -DPAHO_BUILD_DOCUMENTATION=TRUE %cmake_build

%install %cmake_install

- Add CHANGELOG.md CONTRIBUTING.md README.md to %doc

- separate your changelog entries by a new line

- Put the html documenation → documentation

- %{_datadir}/doc/ → %{_docdir}

- No:

%{_libdir}/*

The versioned library (.so.X.x.x) must go to the main package and the unversioned library (.so) must go to the devel package. See https://docs.fedoraproject.org/en-US/packaging-guidelines/#_devel_packages. Do not glob the major soname version of the versioned library while doing this:

%files %license edl-v10 epl-v10 %{_libdir}/libpaho-mqttpp3.so.1*

%files devel %{_includedir}/mqtt %{_libdir}/libpaho-mqttpp3.so

- Be more specific here:

%{_includedir}/mqtt

- This should go to %{_libdir} too not /usr/lib:

/usr/lib/cmake/PahoMqttCpp

Consider sending a patch upstream to fix this.

- The description is too long, it must be wrapped around at 80 characters max per line:

The Paho MQTT CPP Client is a fully fledged MQTT client written in ANSI standard C++ 11.

- If you use cmake3 for EPEL7 (otherwise the 3 is not needed) you should use the cmake macros with 3 too:

%build %cmake3 -DPAHO_WITH_SSL=TRUE -DPAHO_BUILD_DOCUMENTATION=TRUE %cmake3_build

%install %cmake3_install

- The readme file mentions some tests, could you try to run them with %ctest?

- There's an extra 1 at the end here and -p0 is not necessary without the patch:

%autosetup -n paho.mqtt.cpp-%{version} -p0 1

- You need to BuildRequires: gcc-c++, not gcc

- You need to constrain BuildRequires: paho-c-devel >= 1.3.8 for paho-cpp 1.2.0. I have taken the liberty to update it from 1.3.4 to 1.3.9 on Rawhide.

https://bugzilla.redhat.com/show_bug.cgi?id=1964682

--- Comment #3 from Joshua Clayton stillcompiling@gmail.com --- Thank you for that very thorough review.

One item especially, a proper download URL for github is one I've wrestled with and thought I had come up with the best compromise. Thanks for enlightening me.

I'm super pleased that you're bumping paho-c.

I believe I've fixed all the problems you have laid out, with the exception of running the upstream tests as part of build.

I've updated to 1.2.0 (which does not need to be patched), and modified the build process, dependencies and files as requested.

The tests can be made to pass, but they require network access and a running MQTT broker, or some of the tests fail. I have adde the tests to a private branch, and woudl be happy to include them if the caveats can be met. They do take a while, due to testing timeouts.

https://bugzilla.redhat.com/show_bug.cgi?id=1964682

--- Comment #4 from Robert-André Mauchin 🐧 zebob.m@gmail.com --- - Add the license to the devel-docs since it can be installed independently

- The devel-docs subpackage should be noarch:

%package devel-docs Summary: MQTT CPP Client development kit documentation BuildArch: noarch

%description devel-docs Development documentation files for the the Paho MQTT CPP Client.

- %{_libdir}/libpaho-mqttpp3.so.*

As I said before: "Do not glob the major soname version of the versioned library while doing this" We recommend not globbing the major soname version to avoid unintentional soname bump. Soname bump must be declared in advance in the devel mailing list and all the dependent packages must be rebuilt.

%files %license edl-v10 epl-v10 %{_libdir}/libpaho-mqttpp3.so.1*

https://bugzilla.redhat.com/show_bug.cgi?id=1964682

--- Comment #5 from Joshua Clayton stillcompiling@gmail.com --- Added the license line to devel-docs Set the BuildArch of devel-docs to noarch.

Added the explicit "1.*" to the soname. Ah. I misunderstood the nature and purpose of making the major number explicit in the globbing. This will cause rpmbuild to fail in the %files section if we thoughtlessly bump the major version.

I also changed the install target of the cmake files in %prep, (quieting rpmlist) and I made a description change to reflect that the code samples are included in devel-docs, not devel.

https://bugzilla.redhat.com/show_bug.cgi?id=1964682

Joshua Clayton stillcompiling@gmail.com changed:

What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(stillcompiling@gm | |ail.com) |

--- Comment #9 from Joshua Clayton stillcompiling@gmail.com --- Sorry, I missed The fact that this was never imported. It looks like the package would now be up to version 1.3.1, so I'll have to update the submission. Looks like Paho C has been kept up, so that should make it easier.

https://bugzilla.redhat.com/show_bug.cgi?id=1964682

Felix Wang topazus@outlook.com changed:

What |Removed |Added ---------------------------------------------------------------------------- CC| |topazus@outlook.com

--- Comment #10 from Felix Wang topazus@outlook.com ---

Sorry, I missed The fact that this was never imported. It looks like the package would now be up to version 1.3.1, so I'll have to update the submission. Looks like Paho C has been kept up, so that should make it easier.

I am very apologized for this, because I did not look through the package and have already created the paho-cpp package not long ago. I maintained paho-c and paho-cpp package, happy to cooperate with you if you would like to.

https://src.fedoraproject.org/rpms/paho-cpp