Bug 1851405 - Review Request: libbee2 - cryptographic library which implements cryptographic algorithm and protocols standardized in Belarus
Summary: Review Request: libbee2 - cryptographic library which implements cryptographi...
Keywords:
Status: CLOSED NOTABUG
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 FE-DEADREVIEW
TreeView+ depends on / blocked
 
Reported: 2020-06-26 12:57 UTC by Фукидид
Modified: 2021-09-03 00:45 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2021-09-03 00:45:20 UTC
Type: Bug


Attachments (Terms of Use)

Description Фукидид 2020-06-26 12:57:56 UTC
Spec URL: https://copr-be.cloud.fedoraproject.org/results/kashcheyeu/bee2/srpm-builds/01505911/bee2.spec
SRPM URL: https://copr-be.cloud.fedoraproject.org/results/kashcheyeu/bee2/srpm-builds/01505911/bee2-2020.06.24-1.fc32.src.rpm

Description: Bee2 is a cryptographic library which implements cryptographic algorithm and protocols standardized in Belarus. Additionally, Bee2 implements digital signature algorithms standardized in Russia and Ukraine.

Fedora Account System Username: kashcheyeu

https://notabug.org/kashcheyeu/bee2
Bee2 package hosted in Copr: https://copr.fedorainfracloud.org/coprs/kashcheyeu/bee2/

This is my first Fedora package, so I need a sponsor. It would be greatly appreciated if you can sponsor me. Thanks.

Comment 1 Björn 'besser82' Esser 2020-06-29 12:43:02 UTC
Just a few quick remarks from simply looking at the provided spec file:


Summary:	Cryptographic library
Name:		bee2	
Version:	2020.06.24
Release:	1.fc32

***
The release tag MUST use a special macro for the distribution suffix, instead of hardcoding it.

> Release: 1%{?dist}
***

License:	GPLv3
Url:		http://apmi.bsu.by/resources/tools.html
Source0:	https://notabug.org/kashcheyeu/bee2/archive/master.zip
BuildRequires:	cmake, gcc

%description
Bee2 is a cryptographic library which implements cryptographic algorithm and protocols standardized in Belarus. Additionally, Bee2 implements digital signature algorithms standardized in Russia and Ukraine.

***
The %%description of a MUST wrap lines after ~72 chars.

> %description
> Bee2 is a cryptographic library which implements cryptographic algorithm
> and protocols standardized in Belarus.  Additionally, Bee2 implements
> digital signature algorithms standardized in Russia and Ukraine.
***

%package		devel
Summary:		Headers for package bee2
Requires:		%{name} = %{version}-%{release}

***
The devel package MUST have archful requires, if the devel package is archful itself.

> Requires:		%{name}%{?_isa} = %{version}-%{release}
***

%description	devel
Headers for package bee2.

%package		static
Summary:		Static library bee2
%description	static
Static library bee2.

***
The static package MUST require the devel package.

> Requires:		%{name}-devel%{?_isa} = %{version}-%{release}
***

#------------------------------------------------------------------

%prep
%setup -q -n bee2

***
The %%prep stage SHOULD use %%autosetup, if there is no expilcit reason to prefer %%setup.

> %autosetup -n %{name} -p 1
***

%build
%cmake

***
It is highly encouraged to run CMake-based builds out-of-tree.

> %cmake -S %{_vpath_srcdir} -B %{_vpath_builddir}

The %%build stage MUST run %%make_build, if the package builds binaries.

> %make_build -C %{_vpath_builddir}
***

%install
%make_install DESTDIR=%{buildroot}

***
Specifying DESTDIR when using the %%make_install macro is NOT NEEDED.

> %make_install
***

%check
make test

***
Test SHOULD be run in parallel.

> %make_build -C %{_vpath_builddir} test
***

%files

/usr/bin/bsum

***
The path to the installed binaries MUST use macros instead hardcoding them.

> %{_bindir}/bsum
***

%{_libdir}/libbee2.so
%{_libdir}/libbee2_static.a

***
Libraries MUST be shipped in a seperate libs package for enabling multi-arch installation.

The built shared object MUST have a proper so-name, if shipped directly inside of %%{_libdir}.

Static libraries MUST be shipped in a seperate static packge, only.
***

%files static

%{_libdir}/libbee2_static.a

%files devel

/usr/share/bee2/core/b64.h
…

***
Header files for libraries MUST be installed under %%{_includedir}.
***

%changelog
* Fri Jun 26 2020 Yury Kashcheyeu <kashcheyeu@tiksi.ru> - 2020.06.24-1
- Initial RPM releas

***
Spelling error:  releas --> release
***

Comment 2 Björn 'besser82' Esser 2020-06-29 13:58:05 UTC
Another remark:  The sources used for building this package do NOT come from the canonical upstream author!  Nor have there been any releases, yet…

Comment 4 Robert-André Mauchin 🐧 2020-06-30 11:04:36 UTC
 - You must use the Source provided in the upstream official repo: https://github.com/agievich/bee2

 - Are you sure you need the static library, it is not usually shipped.

 - Where does the version you are using is coming from? The upstream repo didn't tag any release.

 - Simplify this:

%{_includedir}/bee2/core/b64.h
%{_includedir}/bee2/core/blob.h
%{_includedir}/bee2/core/dec.h
%{_includedir}/bee2/core/der.h
%{_includedir}/bee2/core/err.h
%{_includedir}/bee2/core/hex.h
%{_includedir}/bee2/core/mem.h
%{_includedir}/bee2/core/mt.h
%{_includedir}/bee2/core/obj.h
%{_includedir}/bee2/core/oid.h
%{_includedir}/bee2/core/prng.h
%{_includedir}/bee2/core/rng.h
%{_includedir}/bee2/core/safe.h
%{_includedir}/bee2/core/stack.h
%{_includedir}/bee2/core/str.h
%{_includedir}/bee2/core/tm.h
%{_includedir}/bee2/core/u16.h
%{_includedir}/bee2/core/u32.h
%{_includedir}/bee2/core/u64.h
%{_includedir}/bee2/core/util.h
%{_includedir}/bee2/core/word.h
%{_includedir}/bee2/crypto/bake.h
%{_includedir}/bee2/crypto/bash.h
%{_includedir}/bee2/crypto/bels.h
%{_includedir}/bee2/crypto/belt.h
%{_includedir}/bee2/crypto/bign.h
%{_includedir}/bee2/crypto/botp.h
%{_includedir}/bee2/crypto/brng.h
%{_includedir}/bee2/crypto/dstu.h
%{_includedir}/bee2/crypto/g12s.h
%{_includedir}/bee2/crypto/pfok.h
%{_includedir}/bee2/defs.h
%{_includedir}/bee2/info.h
%{_includedir}/bee2/math/ec.h
%{_includedir}/bee2/math/ec2.h
%{_includedir}/bee2/math/ecp.h
%{_includedir}/bee2/math/gf2.h
%{_includedir}/bee2/math/gfp.h
%{_includedir}/bee2/math/pp.h
%{_includedir}/bee2/math/pri.h
%{_includedir}/bee2/math/qr.h
%{_includedir}/bee2/math/ww.h
%{_includedir}/bee2/math/zm.h
%{_includedir}/bee2/math/zz.h

Just use %{_includedir}/bee2/ for the whole directory.

 - You must install the license with %license in %files and you should install the %doc as well

%files
%license LICENSE
%doc AUTHORS.md README.md

 - Don't do that:

%if "%dist.%{_arch}" == ".el6.x86_64"

Just use : %if 0%{?rhel} && 0%{?rhel} < 7

You shouldn't need to specify x86_64.

 - Same: %if %{defined fedora}

Just use: %if 0%{?fedora}

 - I'd rather not mix Mageia and Suse stuff with Fedora's spec. Please use separate SPEC if you can.

Comment 6 Robert-André Mauchin 🐧 2020-07-02 13:12:47 UTC
(In reply to Кощеев from comment #5)
> Thanks. Done.
> 
> Version taken from CMakeLists.txt:
> https://github.com/agievich/bee2/blob/master/CMakeLists.txt
> 
> Spec URL:
> https://copr-be.cloud.fedoraproject.org/results/kashcheyeu/bee2/srpm-builds/
> 01516023/bee2.spec
> SRPM URL:
> https://copr-be.cloud.fedoraproject.org/results/kashcheyeu/bee2/srpm-builds/
> 01516023/bee2-2.0.5-4.fc31.src.rpm

As Björn was saying there is a problem with the library, it is not versioned. Fedora requires that library are versioned: see https://docs.fedoraproject.org/en-US/packaging-guidelines/#_shared_libraries


"In cases where upstream ships unversioned .so library (so this is not needed for plugins, drivers, etc.), the packager MUST try to convince upstream to start versioning it.

If that fails due to unwilling or unresponsive upstream, the packager may start versioning downstream but this must be done with caution and ideally only in rare cases. We don’t want to create a library that could conflict with upstream if they later start providing versioned shared libraries. Under no circumstances should the unversioned library be shipped in Fedora.

For downstream versioning, the name should be composed like this:

libfoobar.so.0.n

The n should initially be a small integer (for instance, "1"). we use two digits here ("0.n") because the common practice with upstreams is to use only a single digit here. Using multiple digits helps us avoid potential future conflicts. Do not forget to add the SONAME field (see below) to the library."

You must ask upstream to version their library. There's already commented out code that specify versioning: https://github.com/agievich/bee2/blob/master/src/CMakeLists.txt#L96 If upstream do not wish to start versioning, just comment out that code in your SPEC via a patch.


 - Source must not point to master but to a specific commit if upstream has not released a version:

%global commit          2d8ccce67e5562023309244fe662ee21fd6caf79
%global shortcommit     %(c=%{commit}; echo ${c:0:7})
%global snapshotdate    20200602

Summary:	Cryptographic library
Name:		bee2	
Version:	2.0.5
Release:        4.%{snapshotdate}git%{shortcommit}%{?dist}

License:	GPLv3
Url:		http://apmi.bsu.by/resources/tools.html
Source0:	https://github.com/agievich/bee2/archive/%{commit}/%{name}-%{shortcommit}.tar.gz
BuildRequires:	cmake, gcc, unzip

[…]

%prep
%autosetup -n %{name}-%{commit} -p 1

Comment 8 Robert-André Mauchin 🐧 2020-07-04 08:50:11 UTC
 - The make_install test, if this is supposed to run test, make it run in %check


%install

%if 0%{?fedora}
%make_install -C %{_vpath_builddir}
%else
%make_install
%endif

%{__rm} -rf %{buildroot}%{_libdir}/libbee2_static.a

%check
%make_install -C %{_vpath_builddir} test
%else
%make_install test
%endif

 - Also consider EPEL8 for the condition on CMake:

%if 0%{?fedora} || 0%{?rhel} > 7


The rest looks good.

Comment 9 Björn 'besser82' Esser 2020-07-04 09:43:22 UTC
(In reply to Robert-André Mauchin 🐧 from comment #8)
> The rest looks good.

The package still MUST use %make_build BEFORE running %make_install…

In %check it MUST use %make_build instead of %make_install.

Comment 10 Björn 'besser82' Esser 2020-07-04 09:45:32 UTC
besides that the libraries MUST be shipped in a seperate libs package to resolve conflicts with multi-arch.

Comment 11 Robert-André Mauchin 🐧 2020-07-04 11:29:07 UTC
(In reply to Björn 'besser82' Esser from comment #9)
> (In reply to Robert-André Mauchin 🐧 from comment #8)
> > The rest looks good.
> 
> The package still MUST use %make_build BEFORE running %make_install…
> 
> In %check it MUST use %make_build instead of %make_install.

Ha yes I missed that part.

Comment 13 Robert-André Mauchin 🐧 2020-07-05 13:59:26 UTC
(In reply to Кощеев from comment #12)
> > %if 0%{?fedora} || 0%{?rhel} > 7
> It doesn't work that way:
> https://download.copr.fedorainfracloud.org/results/kashcheyeu/bee2/epel-8-
> x86_64/01517970-bee2/builder-live.log
> 
> Spec URL:
> https://copr-be.cloud.fedoraproject.org/results/kashcheyeu/bee2/fedora-32-
> x86_64/01517971-bee2/bee2.spec
> SRPM URL:
> https://copr-be.cloud.fedoraproject.org/results/kashcheyeu/bee2/srpm-builds/
> 01517971/bee2-2.0.5-6.20200702git2d8ccce.fc32.src.rpm

My bad it requires at least CMake 3.13.

Still, you need to split the libraries into a libs subpackages like Björn 'besser82' Esser asked, or rename the package bee2-libs since you're not shipping any binary anymore.

Comment 15 Фукидид 2020-07-09 11:50:04 UTC
Divided into three packages: bee2 (library), bee2-devel, and bsum (utility). Is everything okay?

Comment 16 Robert-André Mauchin 🐧 2020-07-09 14:22:47 UTC
> bee2 (library)

call it bee-libs
make bee2-devel depends on bee-libs

Comment 18 Robert-André Mauchin 🐧 2020-07-11 16:32:14 UTC
libbee2 is better is there is only one library. Please make the change to the SPEC name and the name in this Review request.

Comment 20 Robert-André Mauchin 🐧 2020-07-12 18:19:59 UTC
The package is approved.

You still need to find a sponsor: see https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group

Comment 21 Фукидид 2020-07-12 20:00:07 UTC
Thank you and Björn 'besser82' Esser, you have helped me a lot.

Comment 22 Mattia Verga 2021-08-03 16:04:24 UTC
Package never imported, resetting ticket status.
If you're still interested and still need to find a sponsor, consider open a ticket in https://pagure.io/packager-sponsors/issues

Comment 23 Package Review 2021-09-03 00:45:20 UTC
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.


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