Bug 1816135

Summary:	Review Request: libjcat - Library for reading Jcat files
Product:	[Fedora] Fedora	Reporter:	Richard Hughes <rhughes>
Component:	Package Review	Assignee:	Kalev Lember <klember>
Status:	CLOSED RAWHIDE	QA Contact:	Fedora Extras Quality Assurance <extras-qa>
Severity:	medium	Docs Contact:
Priority:	medium
Version:	rawhide	CC:	klember, package-review
Target Milestone:	---	Flags:	klember: fedora-review+
Target Release:	---
Hardware:	All
OS:	Linux
Whiteboard:
Fixed In Version:		Doc Type:	If docs needed, set a value
Doc Text:		Story Points:	---
Clone Of:		Environment:
Last Closed:	2020-03-23 13:57:55 UTC	Type:	---
Regression:	---	Mount Type:	---
Documentation:	---	CRM:
Verified Versions:		Category:	---
oVirt Team:	---	RHEL 7.3 requirements from Atomic Host:
Cloudforms Team:	---	Target Upstream Version:
Embargoed:

Description Richard Hughes 2020-03-23 12:14:27 UTC

Spec URL: https://people.freedesktop.org/~hughsient/temp/libjcat.spec
SRPM URL: https://people.freedesktop.org/~hughsient/temp/libjcat-0.1.0-1.fc32.src.rpm
Koji Build: https://koji.fedoraproject.org/koji/taskinfo?taskID=42715759

Description:

This library allows reading and writing gzip-compressed JSON catalog files,
which can be used to store GPG, PKCS-7 and SHA-256 checksums for each file.
`
This provides equivalent functionality to the catalog files supported in
Microsoft Windows.

Fedora Account System Username: rhughes

This will be needed for the next fwupd release as a mandatory build dependency, and the majority of code in this new library are the existing GPG and PKCS-7 helpers in fwupd. As part of fwupd, various security review teams have audited the use of GnuTLS and GpgME with no issues found.

rpmlint output:

$ rpmlint SPECS/libjcat.spec RPMS/libjcat-* SRPMS/libjcat-0.1.0-1.fc32.src.rpm 
libjcat.x86_64: W: spelling-error %description -l en_US gzip -> zip, grip, g zip
libjcat.x86_64: W: spelling-error %description -l en_US checksums -> check sums, check-sums, checks
libjcat.src: W: spelling-error %description -l en_US gzip -> zip, grip, g zip
libjcat.src: W: spelling-error %description -l en_US checksums -> check sums, check-sums, checks
7 packages and 1 specfiles checked; 0 errors, 4 warnings.

Comment 1 Kalev Lember 2020-03-23 12:44:14 UTC

A few notes:

> %{_datadir}/vala/vapi/jcat.deps
> %{_datadir}/vala/vapi/jcat.vapi

You should make sure the directory is included in the package to not cause unowned directories when uninstalling the rpm. Something like:

%dir %{_datadir}/vala
%dir %{_datadir}/vala/vapi

... should take care of it.


> %{_libexecdir}/installed-tests/libjcat/jcat-self-test
> %{_datadir}/installed-tests/libjcat/*
> %dir %{_datadir}/installed-tests/libjcat

Same thing here, need to own %{_datadir}/installed-tests and %{_libexecdir}/installed-tests and %{_libexecdir}/installed-tests/libjcat directories.


> %check
> %meson_test
> 
> %install
> %meson_install

This is not really a problem, but I'd personally reorder the two sections because %check runs after %install.

> %{_datadir}/man/man1/*.1.gz

Can you use a glob such as '*.1*' to avoid hard coding the .gz extension? Flatpak builds for instance use different compression.

rpmlint output:

$ rpmlint libjcat-0.1.0-0.23.20200323git.fc32.src.rpm libjcat-0.1.0-0.23.20200323git.fc32.src.rpm libjcat-devel-0.1.0-0.23.20200323git.fc32.x86_64.rpm libjcat-tests-0.1.0-0.23.20200323git.fc32.x86_64.rpm
libjcat.src: W: spelling-error %description -l en_US gzip -> zip, grip, g zip
libjcat.src: W: spelling-error %description -l en_US checksums -> check sums, check-sums, checks
libjcat.src: W: file-size-mismatch libjcat-0.1.0.tar.xz = 52096, http://people.freedesktop.org/~hughsient/releases/libjcat-0.1.0.tar.xz = 52148
libjcat.src: W: spelling-error %description -l en_US gzip -> zip, grip, g zip
libjcat.src: W: spelling-error %description -l en_US checksums -> check sums, check-sums, checks
libjcat.src: W: file-size-mismatch libjcat-0.1.0.tar.xz = 52096, http://people.freedesktop.org/~hughsient/releases/libjcat-0.1.0.tar.xz = 52148
4 packages and 0 specfiles checked; 0 errors, 6 warnings.

The file size being different sounds like either you updated the tarball or there's a man in the middle attack :) Maybe update the tarball in the srpm and switch to https sources to make MITM harder to do?

These are all just small nitpicks; package APPROVED.

Comment 2 Richard Hughes 2020-03-23 12:48:51 UTC

All nitpicks fixed, thanks for the speedy review.

Comment 3 Gwyn Ciesla 2020-03-23 13:36:06 UTC

(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/libjcat