Bug 1432993 - Review Request: hd-idle - Spin down idle [USB] hard disks
Summary: Review Request: hd-idle - Spin down idle [USB] hard disks
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Randy Barlow
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2017-03-16 14:21 UTC by srakitnican
Modified: 2017-08-15 07:14 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2017-08-15 07:14:08 UTC
Type: ---
Embargoed:
randy: fedora-review+


Attachments (Terms of Use)
review.txt (17.64 KB, text/plain)
2017-08-03 18:38 UTC, Randy Barlow
no flags Details
review-2.txt (17.56 KB, text/plain)
2017-08-07 17:37 UTC, Randy Barlow
no flags Details

Description srakitnican 2017-03-16 14:21:01 UTC
Spec URL: https://drive.google.com/uc?export=download&id=0BzMS3N8RDtOIclg0VUVHbEk1ekE
SRPM URL: https://copr-be.cloud.fedoraproject.org/results/srakitnican/default/fedora-25-x86_64/00519190-hd-idle/hd-idle-1.05-1.fc25.src.rpm

Description:
hd-idle is a utility program for spinning-down external disks after a period
of idle time. Since most external IDE disk enclosures don't support setting
the IDE idle timer, a program like hd-idle is required to spin down idle disks
automatically.

A word of caution: hard disks don't like spinning up too often. Laptop disks
are more robust in this respect than desktop disks but if you set your disks
to spin down after a few seconds you may damage the disk over time due to the
stress the spin-up causes on the spindle motor and bearings. It seems that
manufacturers recommend a minimum idle time of 3-5 minutes, the default in
hd-idle is 10 minutes.

One more word of caution: hd-idle will spin down any disk accessible via the
SCSI layer (USB, IEEE1394, ...) but it will not work with real SCSI disks
because they don't spin up automatically. Thus it's not called scsi-idle and
I don't recommend using it on a real SCSI system unless you have a kernel
patch that automatically starts the SCSI disks after receiving a sense buffer
indicating the disk has been stopped. Without such a patch, real SCSI disks
won't start again and you can as well pull the plug.


Fedora Account System Username:srakitnican

Comment 1 Randy Barlow 2017-07-30 18:16:52 UTC
Hello!

I am a package sponsor and I am willing to review this package for you. Before I begin reviewing your spec file, can you confirm that you still wish to be a packager? I see that this has been open for about four months, so I just want to make sure you are still interested before spending time on it.

Comment 2 Artur Frenszek-Iwicki 2017-07-30 18:28:47 UTC
Randy, it seems that Samuel has been already been sponsored (on two different occasions, even). I'll go ahead and remove the "FE-NEEDSPONSOR" block.
https://bugzilla.redhat.com/show_bug.cgi?id=1457949
https://bugzilla.redhat.com/show_bug.cgi?id=1432983

Comment 3 srakitnican 2017-07-30 21:29:27 UTC
Right, Lubomir Rintel sponsored me on camotics and libdxflib. I guess I don't need a sponsor then.

(In reply to Randy Barlow from comment #1)
> Hello!
> 
> I am a package sponsor and I am willing to review this package for you.
> Before I begin reviewing your spec file, can you confirm that you still wish
> to be a packager? I see that this has been open for about four months, so I
> just want to make sure you are still interested before spending time on it.

Yes, still interested, review would be nice, thank you!

Comment 4 srakitnican 2017-07-30 21:58:22 UTC
I have spotted few things myself that I intend to fix them for the next package release.

 - BuildRequires: gcc
 - Try to use macros for make_build and make_install

Comment 5 Randy Barlow 2017-08-01 16:25:19 UTC
fedora-review is unable to retrieve the spec file URL:

$ fedora-review -b 1432993
INFO: Processing bugzilla bug: 1432993
INFO: Getting .spec and .srpm Urls from : 1432993
INFO:   --> SRPM url: https://copr-be.cloud.fedoraproject.org/results/srakitnican/default/fedora-25-x86_64/00519190-hd-idle/hd-idle-1.05-1.fc25.src.rpm
ERROR: 'Cannot find spec file URL'

Perhaps google drive isn't a suitable host for the spec file? I'm not sure, but I know that you can use your fedorapeople hosting space to host spec files successfully.

Comment 6 srakitnican 2017-08-02 06:40:31 UTC
Sorry for the hassle.

New build on copr 1.05-2 with updated release containing changes mentioned before. Copr now copies spec file along with package.

- Use macros for building and installing
- Add gcc build requirement

Spec URL: https://copr-be.cloud.fedoraproject.org/results/srakitnican/default/fedora-26-x86_64/00585663-hd-idle/hd-idle.spec
SRPM URL: https://copr-be.cloud.fedoraproject.org/results/srakitnican/default/fedora-26-x86_64/00585663-hd-idle/hd-idle-1.05-2.fc26.src.rpm

Comment 7 srakitnican 2017-08-02 06:48:59 UTC
I should mention, there was an issue making package on rawhide currently, not sure why.


error: Empty %files file /builddir/build/BUILD/hd-idle/debugsourcefiles.list
error: Empty %files file /builddir/build/BUILD/hd-idle/debugsourcefiles.list
RPM build errors:
RPM build errors:
    Empty %files file /builddir/build/BUILD/hd-idle/debugsourcefiles.list
    Empty %files file /builddir/build/BUILD/hd-idle/debugsourcefiles.list

Comment 8 srakitnican 2017-08-02 07:53:10 UTC
(In reply to srakitnican from comment #7)
> I should mention, there was an issue making package on rawhide currently,
> not sure why.

Setting %{optflags} might have helped :)


Spec URL: https://copr-be.cloud.fedoraproject.org/results/srakitnican/default/fedora-rawhide-x86_64/00585673-hd-idle/hd-idle.spec
SRPM URL: https://copr-be.cloud.fedoraproject.org/results/srakitnican/default/fedora-rawhide-x86_64/00585673-hd-idle/hd-idle-1.05-3.fc27.src.rpm

Comment 9 Randy Barlow 2017-08-03 18:38:24 UTC
Created attachment 1308835 [details]
review.txt

There are a couple of things that must be fixed for approval, and a couple of optional suggestions. These two must be fixed:

* The package needs to require systemd and logrotate, since it depends on
  directories provided by those packages.
* The changelog's newest entry is for release -2, but the current release is -3.
  Please ensure that there is a changelog entry for the current release.


These are optional:

* fedora-review things that the BuildRequire on gcc is not needed.
* You could easily eliminate the rpmlint warnings:
  - Change scsi to SCSI in the description.
  - Drop the execute bit on the man page.
* I recommend sending a patch to upstream to correct the FSF address in the
  license file. This would fix the rpmlint error.

Comment 10 srakitnican 2017-08-03 19:09:14 UTC
(In reply to Randy Barlow from comment #9)
> * fedora-review things that the BuildRequire on gcc is not needed.

Hmm, Packaging Guidelines says differently, maybe rpmlint is wrong?
    https://fedoraproject.org/wiki/Packaging:C_and_C%2B%2B

> If your application is a C or C++ application you must list a BuildRequires against gcc, gcc-c++ or clang. Those packages will include everything that is required to build a standards conforming C or C++ application.


> * You could easily eliminate the rpmlint warnings:
>   - Change scsi to SCSI in the description.

On a second look, second and third paragraph seems more like documentation then description, how about I drop them completely?

> * I recommend sending a patch to upstream to correct the FSF address in the
>   license file. This would fix the rpmlint error.

I've contacted the author already, I'll see about sending a patch.

Comment 11 srakitnican 2017-08-03 20:41:18 UTC
(In reply to Randy Barlow from comment #9)
> Created attachment 1308835 [details]
> review.txt
> 
> There are a couple of things that must be fixed for approval, and a couple
> of optional suggestions. These two must be fixed:
> 
> * The package needs to require systemd and logrotate, since it depends on
>   directories provided by those packages.

Done

> * The changelog's newest entry is for release -2, but the current release is
> -3.
>   Please ensure that there is a changelog entry for the current release.

Summited the incomplete srpm to soon. The new one should contain all the entries.

> * You could easily eliminate the rpmlint warnings:
>   - Change scsi to SCSI in the description.

scsi in "scsi-idle" seems like a naming reference which is all in lower case. I am not sure changing it is the correct thing to do.


Spec URL: https://copr-be.cloud.fedoraproject.org/results/srakitnican/default/fedora-26-i386/00586353-hd-idle/hd-idle.spec
SRPM URL: https://copr-be.cloud.fedoraproject.org/results/srakitnican/default/fedora-26-i386/00586353-hd-idle/hd-idle-1.05-4.fc26.src.rpm

Comment 12 Randy Barlow 2017-08-07 17:37:19 UTC
Created attachment 1310254 [details]
review-2.txt

Comment 13 Randy Barlow 2017-08-07 17:37:47 UTC
Looks good!

Comment 14 Ralph Bean 2017-08-10 17:00:50 UTC
(fedrepo-req-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/hd-idle

Comment 15 Ralph Bean 2017-08-10 17:01:07 UTC
(fedrepo-req-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/hd-idle

Comment 16 srakitnican 2017-08-15 07:14:08 UTC
Package successfully built for Rawhide, thus closing this. Packages for other releases are planned, but not currently possible to build due to issues with a new SCM.

Thank you.


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