[bug#75568] gnu: smartmontools: Fix PATH in smartd_warning.sh.

Message ID d7320ddeb1939ce0eeeea10f7267883692c269e7.1736898019.git.~@wolfsden.cz
State New
Headers
Series [bug#75568] gnu: smartmontools: Fix PATH in smartd_warning.sh. |

Commit Message

Tomas Volf Jan. 14, 2025, 11:40 p.m. UTC
  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(-)
  

Comments

Hilton Chain Jan. 15, 2025, 2:50 a.m. UTC | #1
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.

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.)

>      (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 Volf Jan. 24, 2025, 3:31 p.m. UTC | #2
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
  
Hilton Chain Jan. 24, 2025, 4:19 p.m. UTC | #3
On Fri, 24 Jan 2025 23:31:56 +0800,
Tomas Volf wrote:
> [...]
> >> +                 (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?

The reason is to record dependency information at <package> level, since this is
where the UI and most of the tools work.  (`guix show', `guix refresh', package
transformation etc.)

For referencing dependencies from inputs in arguments, the benefit is mainly for
transformations.  Maybe it doesn't make much sense in this case (sed and
coreutils), but it's a good practice to follow.
  
Tomas Volf Jan. 28, 2025, 10:11 p.m. UTC | #4
Hilton Chain <hako@ultrarare.space> writes:

> On Fri, 24 Jan 2025 23:31:56 +0800,
> Tomas Volf wrote:
>> [...]
>> >> +                 (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?
>
> The reason is to record dependency information at <package> level, since this is
> where the UI and most of the tools work.  (`guix show', `guix refresh', package
> transformation etc.)

Is there a technical reason it cannot inspect the gexps used or is it
just matter of "no one wrote the code yet"?

>
> For referencing dependencies from inputs in arguments, the benefit is mainly for
> transformations.  Maybe it doesn't make much sense in this case (sed and
> coreutils), but it's a good practice to follow.

I did not realize this at all.  That is very good to know.

Tomas
  
Tomas Volf Jan. 28, 2025, 11:02 p.m. UTC | #5
Hilton Chain <hako@ultrarare.space> writes:

>
> Please add sed and coreutils-minimal to inputs and use search-input-file or
> this-package-input instead.
>
> 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.)

Thank you for the review, I have sent v2 that hopefully incorporates
your feedback.

Tomas
  

Patch

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"))))))))
     (inputs (list libcap-ng))
     (home-page "https://www.smartmontools.org/")
     (synopsis "S.M.A.R.T. harddisk control and monitoring tools")