Bug 1880711 (plasma-disks)
| Summary: | Review Request: plasma-disks - Hard disk health monitoring for KDE Plasma | ||
|---|---|---|---|
| Product: | [Fedora] Fedora | Reporter: | Jan Grulich <jgrulich> |
| Component: | Package Review | Assignee: | Neal Gompa <ngompa13> |
| Status: | CLOSED RAWHIDE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
| Severity: | medium | Docs Contact: | |
| Priority: | medium | ||
| Version: | rawhide | CC: | marcdeop, ngompa13, package-review, rdieter |
| 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: | 2020-09-22 15:42:10 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: | |||
| Bug Depends On: | |||
| Bug Blocks: | 656997 | ||
|
Description
Jan Grulich
2020-09-19 07:30:49 UTC
Is there a reason to create a new variable `%global base_name plasma-disks` when the value is the same as `Name`? (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. (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} ``` Why do we need "%ldconfig_scriptlets"? Unless we're shipping on EL7, it's not needed. Taking this review. 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 This is almost perfect, can you please add "BuildRequires: make" to it, so that we explicitly know the build engine being used? 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" 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. (fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/plasma-disks |