https://bugzilla.redhat.com/show_bug.cgi?id=2017179
Bug ID: 2017179 Summary: Review Request: replxx - Readline and libedit replacement library Product: Fedora Version: rawhide Hardware: All OS: Linux Status: NEW Component: Package Review Severity: medium Assignee: nobody@fedoraproject.org Reporter: minfrin@sharp.fm QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org Target Milestone: --- Classification: Fedora
Spec URL: https://copr.fedorainfracloud.org/coprs/minfrin/replxx/ SRPM URL: https://copr.fedorainfracloud.org/coprs/minfrin/replxx/ Description: A small, portable GNU readline replacement for Linux, Windows and MacOS which is capable of handling UTF-8 characters. Unlike GNU readline, which is GPL, this library uses a BSD license and can be used in any kind of program. Fedora Account System Username: minfrin
https://bugzilla.redhat.com/show_bug.cgi?id=2017179
--- Comment #1 from Graham Leggett minfrin@sharp.fm --- Replxx submission, upstream ticket:
https://github.com/AmokHuginnsson/replxx/issues/125
Disclaimer: I am not the author of the software, nor do I have cmake experience. Will need help addressing the nits in packaging.
https://bugzilla.redhat.com/show_bug.cgi?id=2017179
Alejandro Alvarez a.alvarezayllon@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |a.alvarezayllon@gmail.com Doc Type|--- |If docs needed, set a value
--- Comment #2 from Alejandro Alvarez a.alvarezayllon@gmail.com --- Hello,
From a quick look, I see the following issues:
1. The source code has a LICENSE.md file, which must be included in the rpm
https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuideline...
2. You should also add the licenses of ConvertUTF (no idea which one, mention that is not BSD) and wcwidth (0BSD, I guess?) and mention as a comment which license corresponds to which bit
https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuideline...
ConvertUTF is also part of llvm, and llvm is shipped within Fedora, so I imagine its license is ok
3. The ".so" file (unversioned) must go into the -devel package https://docs.fedoraproject.org/en-US/packaging-guidelines/#_devel_packages
4. The changelog is empty. Add a line indicating this is the initial packaging of replxx or something of the sort
5. The devel package should depend on the main one
https://docs.fedoraproject.org/en-US/packaging-guidelines/#_requiring_base_p...
Minor nitpicks
- Use either spaces or tabs, but try not to mix them
- Use preferably an archive named after the project. It can be done as:
https://github.com/AmokHuginnsson/replxx/archive/release-%%7Bversion%7D/%%7B...
https://bugzilla.redhat.com/show_bug.cgi?id=2017179
--- Comment #3 from Alejandro Alvarez a.alvarezayllon@gmail.com --- Also, you should remove the Group tag https://docs.fedoraproject.org/en-US/packaging-guidelines/#_tags_and_section...
https://bugzilla.redhat.com/show_bug.cgi?id=2017179
--- Comment #4 from Graham Leggett minfrin@sharp.fm --- With licensing, the package is currently stuck behind https://github.com/AmokHuginnsson/replxx/issues/12. Addressing your issues in the mean time, we can hold this until the licensing issue is fixed.
https://bugzilla.redhat.com/show_bug.cgi?id=2017179
--- Comment #5 from Graham Leggett minfrin@sharp.fm --- Found the same code in LLVM, and while the identical license text appears in the LICENSE, no mention is made in the spec file:
https://github.com/llvm/llvm-project/blob/e356027016c6365b3d8924f54c33e2c63d... https://src.fedoraproject.org/rpms/llvm/blob/rawhide/f/llvm.spec
https://bugzilla.redhat.com/show_bug.cgi?id=2017179
--- Comment #6 from Graham Leggett minfrin@sharp.fm --- Created attachment 1841188 --> https://bugzilla.redhat.com/attachment.cgi?id=1841188&action=edit Updates to spec file following review
Attachment contains changes to the spec file.
COPR build is here:
https://copr.fedorainfracloud.org/coprs/minfrin/replxx/
https://bugzilla.redhat.com/show_bug.cgi?id=2017179
Graham Leggett minfrin@sharp.fm changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment|0 |1 #1841188 is| | obsolete| |
--- Comment #7 from Graham Leggett minfrin@sharp.fm --- Created attachment 1841196 --> https://bugzilla.redhat.com/attachment.cgi?id=1841196&action=edit Updates to spec file and license following review
Use output from licensecheck.txt as per https://download.copr.fedorainfracloud.org/results/minfrin/replxx/fedora-raw...
https://bugzilla.redhat.com/show_bug.cgi?id=2017179
--- Comment #8 from Alejandro Alvarez a.alvarezayllon@gmail.com --- Looks better now. However,
(In reply to Graham Leggett from comment #4)
With licensing, the package is currently stuck behind https://github.com/AmokHuginnsson/replxx/issues/12. Addressing your issues in the mean time, we can hold this until the licensing issue is fixed.
I don't think Unicode Strict can be accepted into Fedora, though. So we will have to wait on that issue. I have subscribed as well.
https://bugzilla.redhat.com/show_bug.cgi?id=2017179
Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |zbyszek@in.waw.pl
--- Comment #9 from Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl ---
%{_libdir}/*.so.*
Please specify the SO VERSION here, don't use a glob. See https://docs.fedoraproject.org/en-US/packaging-guidelines/#_listing_shared_l...
I don't think Unicode Strict can be accepted into Fedora, though. So we will have to wait on that issue. I have subscribed as well.
After looking at the original file, the license is problematic. The proposed solution of using the file in LLVM does not be a solution: I think LLVM is in "violation" of the literal license terms too, and they should not say that this file is available under the Apache2 license. I used quotes because it seems that the actual legal risk is negligible. Nevertheless, we want to keep everything kosher in Fedora, so we don't want to accept this minor violation.
utf8cpp seems like a better approach. [1] does a similar conversion, and it seems very straightforward. utf8cpp is in Fedora, so we'd want to just use the system library.
Note that this license was briefly discussed on legal@, but the question was sidestepped because the files weren't actually used [2].
[1] https://github.com/taglib/taglib/pull/794/files [2] https://lists.fedoraproject.org/archives/list/legal@lists.fedoraproject.org/...
https://bugzilla.redhat.com/show_bug.cgi?id=2017179
--- Comment #10 from Graham Leggett minfrin@sharp.fm --- I've updated https://github.com/AmokHuginnsson/replxx/issues/12 with these details.
https://bugzilla.redhat.com/show_bug.cgi?id=2017179
Graham Leggett minfrin@sharp.fm changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(minfrin@sharp.fm) |needinfo?(zbyszek@in.waw.pl | |)
--- Comment #12 from Graham Leggett minfrin@sharp.fm ---
%{_libdir}/*.so.*
Please specify the SO VERSION here, don't use a glob. See https://docs.fedoraproject.org/en-US/packaging-guidelines/#_listing_shared_l...
The cmake config for this is beyond me, are you able to help?
On Mageia etc, the libraries are called libreplxx-rd.so.0.0.4 (note the extra "-rd") and I cannot for the life of me find a working regex to allow "libreplxx" or "libreplxx-rd".
In addition, it appears that cmake is using the VERSION number as the SOVERSION number, but I don't know enough about cmake to know the correct way to declare a SOVERSION.
Can you clarify if possible?
Product: Fedora Version: rawhide Component: Package Review
Graham Leggett minfrin@sharp.fm has canceled Package Review package-review@lists.fedoraproject.org's request for Graham Leggett minfrin@sharp.fm's needinfo: Bug 2017179: Review Request: replxx - Readline and libedit replacement library https://bugzilla.redhat.com/show_bug.cgi?id=2017179
--- Comment #12 from Graham Leggett minfrin@sharp.fm ---
%{_libdir}/*.so.*
Please specify the SO VERSION here, don't use a glob. See
https://docs.fedoraproject.org/en-US/packaging-guidelines/#_listing_shared_l... ary_files
The cmake config for this is beyond me, are you able to help?
On Mageia etc, the libraries are called libreplxx-rd.so.0.0.4 (note the extra "-rd") and I cannot for the life of me find a working regex to allow "libreplxx" or "libreplxx-rd".
In addition, it appears that cmake is using the VERSION number as the SOVERSION number, but I don't know enough about cmake to know the correct way to declare a SOVERSION.
Can you clarify if possible?
https://bugzilla.redhat.com/show_bug.cgi?id=2017179
--- Comment #13 from Graham Leggett minfrin@sharp.fm --- The license issue has been fixed here: https://github.com/AmokHuginnsson/replxx/pull/142
The latest RPM build at https://copr.fedorainfracloud.org/coprs/minfrin/replxx/packages/ includes the above patch.
https://bugzilla.redhat.com/show_bug.cgi?id=2017179
--- Comment #14 from Graham Leggett minfrin@sharp.fm --- Quick ping to revive this.
The author of replxx appears to be no longer responsive, is it possible to apply the patch at https://github.com/AmokHuginnsson/replxx/pull/142 as part of the package build to solve the license issue?
https://bugzilla.redhat.com/show_bug.cgi?id=2017179
Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(zbyszek@in.waw.pl | |) | Assignee|nobody@fedoraproject.org |zbyszek@in.waw.pl Status|NEW |ASSIGNED
--- Comment #15 from Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl ---
The author of replxx appears to be no longer responsive, is it possible to apply the patch at https://github.com/AmokHuginnsson/replxx/pull/142 as part of the package build to solve the license issue?
I think so. Upstream made a mistake in the licensing annotation, but we have clarified that actual license is more permissive, so we can use the file.
No big comments, but some suggestions:
Release: 3%{?dist}
'Release: %autorelease' and '%autochangelog' instead of an explicit changelog, see https://docs.pagure.org/fedora-infra.rpmautospec/opting-in.html
BuildRequires: cmake, gcc, gcc-c++
Please use separate lines for each of the items.
%setup -q %patch0 -p1
%autosetup -p1
%cmake .
Drop the '.'. https://docs.fedoraproject.org/en-US/packaging-guidelines/CMake/ don't use it.
ctest -V %{?_smp_mflags}
Just '%ctest', see https://docs.fedoraproject.org/en-US/packaging-guidelines/CMake/.
%{_libdir}/*.so
%{_libdir}/libreplxx*.so
The directory %{_datadir}/cmake/%{name} should be owned by the package. So change
%{_datadir}/cmake/%{name}/*.cmake
to '%{_datadir}/cmake/%{name}'.
%doc
Change that to '%doc README.md'.
Hmm, when I download the specified Source0 file, the top directory is called 'replxx-release-0.0.4/'. So it sounds like you need '%autosetup -p1 -n replxx-release-%{version}'
%description should be wrapped to <= 80 columns (right now it's ~65).
This is pretty close, but let's do another round because of the various small nitpicks.
https://bugzilla.redhat.com/show_bug.cgi?id=2017179
--- Comment #16 from Graham Leggett minfrin@sharp.fm --- Thank you for this.
I have another build at https://copr.fedorainfracloud.org/coprs/minfrin/replxx/build/6600722/ - can you take a look?
Release: 3%{?dist}
'Release: %autorelease' and '%autochangelog' instead of an explicit changelog, see
https://docs.pagure.org/fedora-infra.rpmautospec/opting-in.html
I wasn't able to get this to work, working from a simple rpmbuild/SPECS file directory right now, I believe %autorelease needs git?
BuildRequires: cmake, gcc, gcc-c++
Please use separate lines for each of the items.
Done.
%setup -q %patch0 -p1
%autosetup -p1
Done.
%cmake .
Drop the '.'. https://docs.fedoraproject.org/en-US/packaging-guidelines/CMake/ don't use it.
Done.
ctest -V %{?_smp_mflags}
Just '%ctest', see https://docs.fedoraproject.org/en-US/packaging-guidelines/CMake/.
Done.
%{_libdir}/*.so
%{_libdir}/libreplxx*.so
Done.
The directory %{_datadir}/cmake/%{name} should be owned by the package. So change
%{_datadir}/cmake/%{name}/*.cmake
to '%{_datadir}/cmake/%{name}'.
Done.
%doc
Change that to '%doc README.md'.
Done.
Hmm, when I download the specified Source0 file, the top directory is called 'replxx-release-0.0.4/'. So it sounds like you need '%autosetup -p1 -n replxx-release-%{version}'
Done.
%description should be wrapped to <= 80 columns (right now it's ~65).
Done.
https://bugzilla.redhat.com/show_bug.cgi?id=2017179
--- Comment #17 from Graham Leggett minfrin@sharp.fm --- More polishing: https://copr.fedorainfracloud.org/coprs/minfrin/replxx/build/6600844/
https://bugzilla.redhat.com/show_bug.cgi?id=2017179
Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |POST Flags|fedora-review? |fedora-review+
--- Comment #18 from Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl --- Oh, the license string should be SPDX: https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuideline... BSD-3-Clause AND 0BSD AND Unicode-DFS-2015
+ package name is OK + latest version + license is acceptable for Fedora (BSD-3-Clause + some others) - license is specified correctly (see above) + builds and installs OK + P/R/BR look OK
Package is APPROVED.
I'd still recommend switching to rpmautospec after importing into the git repo.
https://bugzilla.redhat.com/show_bug.cgi?id=2017179
--- Comment #19 from Graham Leggett minfrin@sharp.fm --- I just fixed the license here: https://copr.fedorainfracloud.org/coprs/minfrin/replxx/build/6973489/
https://bugzilla.redhat.com/show_bug.cgi?id=2017179
--- Comment #20 from Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl --- The package was already approved, to you should now request the repo and do a build.
package-review@lists.fedoraproject.org
-
bugzilla＠redhat.com