https://bugzilla.redhat.com/show_bug.cgi?id=1964682
Bug ID: 1964682 Summary: Review Request: paho-cpp - Eclipse Paho MQTT C++ client api Product: Fedora Version: rawhide Hardware: All OS: Linux Status: NEW Component: Package Review Severity: medium Assignee: nobody@fedoraproject.org Reporter: stillcompiling@gmail.com QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org Target Milestone: --- Classification: Fedora
Spec URL: https://raw.githubusercontent.com/d4ddi0/paho.mqtt.cpp/5ac0f2fb9546f6f8d1904...
https://raw.githubusercontent.com/d4ddi0/paho.mqtt.cpp/5ac0f2fb9546f6f8d1904... SRPM URL: https://drive.google.com/file/d/13lFm0qjXSyWUQmfvEVUPWxCVm0_RtjO_/view?usp=s... Description: Paho MQTT C++ Asynchronous C++ api for communicating with an MQTT broker 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 Fedora Account System Username: daddio
$ rpmlint SPECS/paho-cpp-1.1.spec SPECS/paho-cpp-1.1.spec:65: E: hardcoded-library-path in /usr/lib/cmake/PahoMqttCpp SPECS/paho-cpp-1.1.spec: W: invalid-url Source0: https://github.com/eclipse/paho.mqtt.cpp/archive/v-1.1.tar.gz HTTP Error 404: Not Found 0 packages and 1 specfiles checked; 1 errors, 1 warnings.
It would be nice to have paho-cpp in-tree.
Product: Fedora Version: rawhide Component: Package Review
Joshua Clayton stillcompiling@gmail.com has asked Package Review package-review@lists.fedoraproject.org for needinfo: Bug 1964682: Review Request: paho-cpp - Eclipse Paho MQTT C++ client api https://bugzilla.redhat.com/show_bug.cgi?id=1964682
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
Joshua Clayton stillcompiling@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |177841 (FE-NEEDSPONSOR)
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=177841 [Bug 177841] Tracker: Review requests from new Fedora packagers who need a sponsor
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 #2 from Robert-André Mauchin 🐧 zebob.m@gmail.com ---
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.
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
Joshua Clayton stillcompiling@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Comment|0 |updated
--- Comment #0 has been edited ---
Spec URL: https://raw.githubusercontent.com/d4ddi0/paho.mqtt.cpp/e0df577b59d9ec8456b4f...
SRPM URL: https://drive.google.com/file/d/1y9HHWrbdWXpgjW_MBHLJu65PAdj91tLg/view?usp=s... 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
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
Joshua Clayton stillcompiling@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Comment|0 |updated
--- Comment #0 has been edited ---
Spec URL: https://raw.githubusercontent.com/d4ddi0/paho.mqtt.cpp/87478d9b771080adf3b8b...
SRPM URL: https://drive.google.com/file/d/1y9HHWrbdWXpgjW_MBHLJu65PAdj91tLg/view?usp=s... 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
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
Robert-André Mauchin 🐧 zebob.m@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |POST Assignee|nobody@fedoraproject.org |zebob.m@gmail.com Flags| |fedora-review+
--- Comment #6 from Robert-André Mauchin 🐧 zebob.m@gmail.com ---
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/87478d9b771080adf3b8b... is approved.
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.
Product: Fedora Version: rawhide Component: Package Review
Joshua Clayton stillcompiling@gmail.com has canceled Package Review package-review@lists.fedoraproject.org's request for Joshua Clayton stillcompiling@gmail.com's needinfo: Bug 1964682: Review Request: paho-cpp - Eclipse Paho MQTT C++ client api https://bugzilla.redhat.com/show_bug.cgi?id=1964682
--- 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
https://bugzilla.redhat.com/show_bug.cgi?id=1964682
Joshua Clayton stillcompiling@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |UPSTREAM Status|NEW |CLOSED Last Closed| |2024-01-16 03:26:51
--- Comment #11 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.
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.
@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)
package-review@lists.fedoraproject.org
-
bugzilla＠redhat.com