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 1964682 - Review Request: paho-cpp - Eclipse Paho MQTT C++ client api
Summary: Review Request: paho-cpp - Eclipse Paho MQTT C++ client api
Keywords:
Status: NEW
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Nobody's working on this, feel free to take it
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: FE-NEEDSPONSOR
TreeView+ depends on / blocked
 
Reported: 2021-05-25 20:19 UTC by Joshua Clayton
Modified: 2021-06-03 22:22 UTC (History)
3 users (show)

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


Attachments (Terms of Use)

Description Joshua Clayton 2021-05-25 20:19:02 UTC
Spec URL: https://raw.githubusercontent.com/d4ddi0/paho.mqtt.cpp/87478d9b771080adf3b8b06a03aa42571570c841/dist/paho-cpp.spec

SRPM URL: https://drive.google.com/file/d/1y9HHWrbdWXpgjW_MBHLJu65PAdj91tLg/view?usp=sharing
Description: Paho MQTT C++
Asynchronous C++ api for communicating with an MQTT broker
The linked spec is for version 1.2.0
requires paho-c 1.3.8, which is not yet current in fedora

Fedora Account System Username: daddio

$ rpmlint paho-cpp.spec 
paho-cpp.spec:53: E: hardcoded-library-path in %{buildroot}/usr/lib/cmake/PahoMqttCpp
0 packages and 1 specfiles checked; 1 errors, 0 warnings.


It would be nice to have paho-cpp in-tree.
Updated URLs for paho-cpp 1.2.0

Comment 1 Robert-André Mauchin 🐧 2021-05-29 14:33:07 UTC
 - 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.

Comment 2 Robert-André Mauchin 🐧 2021-05-29 14:36:10 UTC
> 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.

Comment 3 Joshua Clayton 2021-06-01 22:20:39 UTC
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.

Comment 4 Robert-André Mauchin 🐧 2021-06-02 16:08:05 UTC
 - 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*

Comment 5 Joshua Clayton 2021-06-03 22:22:26 UTC
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.


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