Subject: Re: [bug#50325] [PATCH] gnu: Add fatrace.

From: Raphaël Mélotte <[email-masked]>
Date: Wed, 1 Sep 2021 22:34:49 +0200

Thanks for the review!

On 9/1/21 10:10 PM, Xinglu Chen wrote:
> On Wed, Sep 01 2021, Raphaël Mélotte wrote:
> 
>> * gnu/packages/monitoring.scm (fatrace): New variable.
>> ---
>>   gnu/packages/monitoring.scm | 28 ++++++++++++++++++++++++++++
>>   1 file changed, 28 insertions(+)
>>
>> diff --git a/gnu/packages/monitoring.scm b/gnu/packages/monitoring.scm
>> index d2aef0eaab..87df0fa634 100644
>> --- a/gnu/packages/monitoring.scm
>> +++ b/gnu/packages/monitoring.scm
>> @@ -9,6 +9,7 @@
>>   ;;; Copyright © 2020 Lars-Dominik Braun <[email-masked]>
>>   ;;; Copyright © 2021 Marius Bakke <[email-masked]>
>>   ;;; Copyright © 2021 Stefan Reichör <[email-masked]>
>> +;;; Copyright © 2021 Raphaël Mélotte <[email-masked]>
>>   ;;;
>>   ;;; This file is part of GNU Guix.
>>   ;;;
>> @@ -650,3 +651,30 @@ LAN, if wanted, and clients can switch between multiple hosts on the network.
>>   Hostscope features a bridge to Influx DB.  So Grafana can be used to visualize
>>   the recorded data over time.")
>>       (license license:gpl3+)))
>> +
>> +(define-public fatrace
>> +  (package
>> +    (name "fatrace")
>> +    (version "0.16.3")
>> +    (source (origin
>> +              (method git-fetch)
>> +              (uri (git-reference
>> +                    (url "https://github.com/martinpitt/fatrace")
>> +                    (commit version)))
>> +              (file-name (git-file-name name version))
>> +              (sha256
>> +               (base32
>> +                "1bxz6v1z0icp716jnv3knjyqp8bv6xnkz8gqd8z3g2b6yxj5xff3"))))
>> +    (build-system gnu-build-system)
>> +    (arguments
>> +     `(#:phases (modify-phases %standard-phases
> 
> Nit: The ‘modify-phases’ form is usually on a separate line:
> 
>    (arguments
>     `(#:phases
>       (modify-phases %standard-phases
>        ...)))
>        

Ok, I'll change it.

>> +                  (delete 'configure)
>> +                  ;; there are no tests to execute:
>> +                  (delete 'check))
> 
> Are you sure about that?  There is a test/ directory in the Git repo.

Indeed, the comment is incorrect.
That said the tests need to run as root, have no make target, and need write 
access to /etc so I think we can't run them anyway (at least not as-is).

> 
>> +       #:make-flags
>> +       (list (string-append "CC=" ,(cc-for-target))
>> +             (string-append "PREFIX=" %output))))
>> +    (synopsis "File access events monitor")
>> +    (description "Utility to report system wide file access events.")
> 
> The description should be a complete sentence; maybe (based on the README)
> 
>    This package provides a utility to report system wide file access
>    events from all running processes.  Its main purpose is to find
>    processes which keep waking up the disk unnecessarily and thus prevent
>    some power saving.
> 

Ok, I'll update it.