Bug 2213779

Summary: Review Request: kjournald - Framework for interacting with systemd-journald
Product: [Fedora] Fedora Reporter: Steve Cossette <farchord>
Component: Package ReviewAssignee: Neal Gompa <ngompa13>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: ngompa13, package-review, topazus
Target Milestone: ---Flags: ngompa13: 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: 2023-06-25 20:08:27 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 Steve Cossette 2023-06-09 11:12:00 UTC
Spec URL: https://api.nicehomeservices.com/images/temp/kjournald.spec
SRPM URL: https://api.nicehomeservices.com/images/temp/kjournald-23.04.2-1.fc39.src.rpm
Description: Framework for interacting with systemd-journald
Fedora Account System Username: farchord

Comment 1 Neal Gompa 2023-06-09 21:04:07 UTC
Taking this review.

Comment 2 Neal Gompa 2023-06-09 21:07:27 UTC
Initial spec review:

> %autosetup -n %{name}-%{version} -p1

The "-n %{name}-%{version}" is superfluous and can be dropped.

> %{cmake_kf5}

You can drop the curly braces here.

> %{_libdir}/libkjournald.so
> %{_libdir}/libkjournald.so.23
> %{_libdir}/libkjournald.so.23.04.2

This needs to be broken out to a -libs subpackage. The library seems to be versioned by the software version, so you'll want macros to track that.

For example, add the following to the top of the spec:

%global majorver 23
%global fullver %{majorver}.04.2

Then change the Version as follows:

Version: %{fullver}

Then change the file list as follows:

%{_libdir}/libkjournald.so
%{_libdir}/libkjournald.so.%{majorver}
%{_libdir}/libkjournald.so.%{fullver}

Comment 3 Steve Cossette 2023-06-09 21:50:29 UTC
Changes made to the spec, and reuploaded the .spec and the .src.rpm to the server.

I used dolphin as an example of how to split libraries in a separate package (https://src.fedoraproject.org/rpms/dolphin/blob/rawhide/f/dolphin.spec), was wondering what the "%ldconfig_scriptlets libs" line did. I noticed after seeing https://asamalik.fedorapeople.org/tmp-docs-preview/packaging-guidelines/Scriptlets/ that it's no longer needed in the recent versions of Fedora, so I omitted it.

Comment 4 Felix Wang 2023-06-12 07:33:03 UTC
The dekstop file and appdata file need to be validated according to the fedora packaging guidelines.

ref: https://docs.fedoraproject.org/en-US/packaging-guidelines/AppData/
https://docs.fedoraproject.org/en-US/packaging-guidelines/#_desktop_files

Comment 5 Steve Cossette 2023-06-12 10:54:56 UTC
(In reply to Felix Wang from comment #4)
> The dekstop file and appdata file need to be validated according to the
> fedora packaging guidelines.
> 
> ref: https://docs.fedoraproject.org/en-US/packaging-guidelines/AppData/
> https://docs.fedoraproject.org/en-US/packaging-guidelines/#_desktop_files

Yep that's my bad, I copied a spec from another project and for whatever reason I deleted those lines... Added them back and updated the spec/srpm on the server.

Comment 6 Neal Gompa 2023-06-25 18:38:08 UTC
Review notes:

* Packaging follows Fedora KDE packaging
* Packaging follows Fedora packaging guidelines
* Package builds and installs
* Package licensing is follows our guidelines

Looks good to me.

PACKAGE APPROVED.

Comment 7 Fedora Admin user for bugzilla script actions 2023-06-25 19:48:30 UTC
The Pagure repository was created at https://src.fedoraproject.org/rpms/kjournald