Bug 1880711 (plasma-disks) - Review Request: plasma-disks - Hard disk health monitoring for KDE Plasma
Summary: Review Request: plasma-disks - Hard disk health monitoring for KDE Plasma
Keywords:
Status: CLOSED RAWHIDE
Alias: plasma-disks
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Neal Gompa
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: kde-reviews
TreeView+ depends on / blocked
 
Reported: 2020-09-19 07:30 UTC by Jan Grulich
Modified: 2020-09-22 15:42 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2020-09-22 15:42:10 UTC
Type: ---
Embargoed:
ngompa13: fedora-review+


Attachments (Terms of Use)

Description Jan Grulich 2020-09-19 07:30:49 UTC
Spec URL: https://jgrulich.fedorapeople.org/plasma-disks/plasma-disks.spec
SRPM URL: https://jgrulich.fedorapeople.org/plasma-disks/plasma-disks-5.19.90-1.fc32.src.rpm
Description: 
Plasma Disks monitors S.M.A.R.T. data of disks and alerts the user when
signs of imminent failure appear.
Fedora Account System Username: jgrulich

Comment 1 marcdeop 2020-09-22 12:44:54 UTC
Is there a reason to create a new variable `%global base_name    plasma-disks` when the value is the same as `Name`?

Comment 2 Jan Grulich 2020-09-22 12:52:40 UTC
(In reply to marcdeop from comment #1)
> Is there a reason to create a new variable `%global base_name   
> plasma-disks` when the value is the same as `Name`?

It is kind of useless in this case, but we have been using this across all Plasma components, because in certain cases the tarball name doesn't match the package name. I kept it here for consistency.

Comment 3 marcdeop 2020-09-22 12:56:03 UTC
(In reply to Jan Grulich from comment #2)
> (In reply to marcdeop from comment #1)
> > Is there a reason to create a new variable `%global base_name   
> > plasma-disks` when the value is the same as `Name`?
> 
> It is kind of useless in this case, but we have been using this across all
> Plasma components, because in certain cases the tarball name doesn't match
> the package name. I kept it here for consistency.

I don't mean to be offensive and maybe I am being pedantic but: wouldn't it make sense then to always use the `base_name` variable instead of sometimes `name` and sometimes `base_name`?

For instance, from your spec:

```
%find_lang %{name} --all-name
%files -f %{name}.lang
```

but:
```
%autosetup -n %{base_name}-%{version} -p1
Source0:        http://download.kde.org/%{stable}/plasma/%{version}/%{base_name}-%{version}.tar.xz
URL:     https://invent.kde.org/plasma/%{base_name}
```

Comment 4 Neal Gompa 2020-09-22 13:10:32 UTC
Why do we need "%ldconfig_scriptlets"? Unless we're shipping on EL7, it's not needed.

Comment 5 Neal Gompa 2020-09-22 13:14:34 UTC
Taking this review.

Comment 6 Jan Grulich 2020-09-22 13:32:39 UTC
Spec URL: https://jgrulich.fedorapeople.org/plasma-disks/plasma-disks.spec
SRPM URL: https://jgrulich.fedorapeople.org/plasma-disks/plasma-disks-5.19.90-1.fc32.src.rpm
Description: 
Plasma Disks monitors S.M.A.R.T. data of disks and alerts the user when
signs of imminent failure appear.
Fedora Account System Username: jgrulich

1) Removed %ldconfig_scriptlets
2) Removed %base_name define

Comment 7 Neal Gompa 2020-09-22 14:34:52 UTC
This is almost perfect, can you please add "BuildRequires: make" to it, so that we explicitly know the build engine being used?

Comment 8 Jan Grulich 2020-09-22 14:59:25 UTC
Spec URL: https://jgrulich.fedorapeople.org/plasma-disks/plasma-disks.spec
SRPM URL: https://jgrulich.fedorapeople.org/plasma-disks/plasma-disks-5.19.90-1.fc32.src.rpm
Description: 
Plasma Disks monitors S.M.A.R.T. data of disks and alerts the user when
signs of imminent failure appear.
Fedora Account System Username: jgrulich

1) Added "BuildRequires: make"

Comment 9 Neal Gompa 2020-09-22 15:18:16 UTC
Review notes:

* Package is named appropriately
* Package builds and installs properly
* Package licensing is correct and license files are correctly installed
* Package has correct desktop and AppStream metainfo files

PACKAGE APPROVED.

Comment 10 Gwyn Ciesla 2020-09-22 15:31:55 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/plasma-disks


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