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
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.
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/LicensingGuidelines/#_license_text 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/LicensingGuidelines/#_multiple_licensing_scenarios 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_package 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-%{version}/%{name}-%{version}.tar.gz
Also, you should remove the Group tag https://docs.fedoraproject.org/en-US/packaging-guidelines/#_tags_and_sections
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.
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/e356027016c6365b3d8924f54c33e2c63d931492/llvm/include/llvm/Support/ConvertUTF.h https://src.fedoraproject.org/rpms/llvm/blob/rawhide/f/llvm.spec
Created attachment 1841188 [details] Updates to spec file following review Attachment contains changes to the spec file. COPR build is here: https://copr.fedorainfracloud.org/coprs/minfrin/replxx/
Created attachment 1841196 [details] Updates to spec file and license following review Use output from licensecheck.txt as per https://download.copr.fedorainfracloud.org/results/minfrin/replxx/fedora-rawhide-x86_64/02950145-replxx/fedora-review/licensecheck.txt
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.
> %{_libdir}/*.so.* Please specify the SO VERSION here, don't use a glob. See https://docs.fedoraproject.org/en-US/packaging-guidelines/#_listing_shared_library_files > 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/thread/KQEAGD4UCDRO2X5P4WYOOT5FUFWXJ4NP/#KQEAGD4UCDRO2X5P4WYOOT5FUFWXJ4NP
I've updated https://github.com/AmokHuginnsson/replxx/issues/12 with these details.
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.
> > %{_libdir}/*.so.* > > Please specify the SO VERSION here, don't use a glob. See > https://docs.fedoraproject.org/en-US/packaging-guidelines/#_listing_shared_library_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?
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.
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?
> 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.
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.
More polishing: https://copr.fedorainfracloud.org/coprs/minfrin/replxx/build/6600844/
Oh, the license string should be SPDX: https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuidelines/#_valid_license_short_names 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.
I just fixed the license here: https://copr.fedorainfracloud.org/coprs/minfrin/replxx/build/6973489/
The package was already approved, to you should now request the repo and do a build.