- 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%{version}/%{name}-%{version}.tar.gz

 - 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_package
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.

> The linked spec is for version 1.1, which workes with in-tree
paho-c 1.3.4. I also have a spec for paho-cpp v1.2.0 (later version of the same github repo), but it requires paho-c 1.3.8, which is not yet in fedora 

Sorry I missed that. I've updated paho-c using my privileges.

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.

 - 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*

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.

> 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.

That is the point. Soname bump must be announced one week in advance on the devel mailing list and be done silently to avoid breaking other packages which depend on the old soname.

Package https://raw.githubusercontent.com/d4ddi0/paho.mqtt.cpp/87478d9b771080adf3b8b06a03aa42571570c841/dist/paho-cpp.spec is approved.

Package was never imported, resetting ticket status.

This is an automatic check from review-stats script.

This review request ticket hasn't been updated for some time. We're sorry
it is taking so long. If you're still interested in packaging this software
into Fedora repositories, please respond to this comment clearing the
NEEDINFO flag.

You may want to update the specfile and the src.rpm to the latest version
available and to propose a review swap on Fedora devel mailing list to increase
chances to have your package reviewed. If this is your first package and you
need a sponsor, you may want to post some informal reviews. Read more at
https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group.

Without any reply, this request will shortly be considered abandoned
and will be closed.
Thank you for your patience.

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.

> 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

> > 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

@Felix Wang, Thanks for creating the package. I'm delighted that the paho-cpp is now available through dnf. I'm just an ordinary user of the library. (In reply to Felix Wang from comment #10)