Thank you for the review, responses below.

Hilton Chain <hako@ultrarare.space> writes:

> Hi Tomas,
>
> On Wed, 15 Jan 2025 07:40:19 +0800,
> Tomas Volf wrote:
>>
>> The script started with reset of the $PATH to a value not suitable to Guix.
>> In addition, the script requires coreutils and sed, so add those into the
>> $PATH.
>>
>> * gnu/packages/admin.scm (smartmontools)[arguments]<#:phases>: Add 'fix-path.
>>
>> Change-Id: Ide97f572e6f369fe24337f945474dc7a65584eda
>> ---
>>  gnu/packages/admin.scm | 15 +++++++++++++--
>>  1 file changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/gnu/packages/admin.scm b/gnu/packages/admin.scm
>> index 7f50d5f4e9..098e21ff8a 100644
>> --- a/gnu/packages/admin.scm
>> +++ b/gnu/packages/admin.scm
>> @@ -2921,8 +2921,19 @@ (define-public smartmontools
>>                  "0gcrzcb4g7f994n6nws26g6x15yjija1gyzd359sjv7r3xj1z9p9"))))
>>      (build-system gnu-build-system)
>>      (arguments
>> -     (list #:make-flags
>> -           #~(list "BUILD_INFO=\"(Guix)\"")))
>> +     (list
>> +      #:make-flags
>> +      #~(list "BUILD_INFO=\"(Guix)\"")
>> +      #:phases
>> +      #~(modify-phases %standard-phases
>> +          (add-after 'install 'fix-path
>> +            (lambda _
>> +              (substitute* (string-append #$output "/etc/smartd_warning.sh")
>> +                (("export PATH=.*$" all)
>> +                 (string-append "PATH="
>> +                                #$(file-append sed "/bin") ":"
>> +                                #$(file-append coreutils "/bin") ":"
>> +                                "$PATH\n"))))))))
>
> Please add sed and coreutils-minimal to inputs and use search-input-file or
> this-package-input instead.

Why?  I will of course do so, but would first like to understand why.
The current way seems to work and keeps all the references in just one
place.  If I switch to the suggested approach, both `sed' and
`coreutils' will be referenced in two places (once in the `inputs' and
once here) and those will need to be kept in sync.  So, uh, why do it?

>
> For smartmontools the proper way is to set --with-scriptpath='...' configure
> flag, which is documented in its INSTALL file.  (It can be disabled with a 'no'
> value as well.)

This does indeed seem to be more elegant approach, will use it for v2.

>
>>      (inputs (list libcap-ng))
>>      (home-page "https://www.smartmontools.org/")
>>      (synopsis "S.M.A.R.T. harddisk control and monitoring tools")
>> --
>> 2.47.1
>
> Thanks

Tomas

-- 
There are only two hard things in Computer Science:
cache invalidation, naming things and off-by-one errors.