Bug 2017179

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.

Package was never imported.
The ticket status is being reset, since creating the repository will require a fresh approval.
Let us know if you're still interested in this package.

This is an automatic action taken by review-stats script.

The ticket submitter failed to clear the NEEDINFO flag in a month.
As per https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews
we consider this ticket as DEADREVIEW and proceed to close it.