Subject: Re: bug#59762: Add serial-studio

From: Maxim Cournoyer <[email-masked]>
Date: Tue, 21 Mar 2023 09:53:03 -0400

Hello,

Maxim Cournoyer <[email-masked]> writes:

> Hi again!
>
> phodina <[email-masked]> writes:
>
>> Hi,
>>
>> this adds a nice GUI program to monitor communication on serial interfaces (e.g. UART).
>>
>> ----
>> Petr
>>
>>
>>
>> From 6b25d088bd158ad2c5baa7548c49a04bd0f09664 Mon Sep 17 00:00:00 2001
>> From: Petr Hodina <[email-masked]>
>> Date: Wed, 19 Jan 2022 12:01:22 +0100
>> Subject: [PATCH 2/4] gnu: Add qtcsv.
>>
>> * gnu/packages/qt.scm (qtcsv): New variable.
>>
>> diff --git a/gnu/packages/qt.scm b/gnu/packages/qt.scm
>> index 6aaa9c47c2..71c8aafe3c 100644
>> --- a/gnu/packages/qt.scm
>> +++ b/gnu/packages/qt.scm
>> @@ -1283,6 +1283,58 @@ (define-public qtconnectivity
>>      (description "The Qt Connectivity modules provides modules for interacting
>>  with Bluetooth and NFC.")))
>>  
>> +(define-public qtcsv
>> +  (package
>> +    (name "qtcsv")
>> +    (version "1.6")
>> +    (source (origin
>> +              (method git-fetch)
>> +              (uri (git-reference
>> +                    (url "https://github.com/iamantony/qtcsv")
>> +                    (commit (string-append "v" version))))
>> +              (file-name (git-file-name name version))
>> +              (sha256
>> +               (base32
>> +                "109nbspnhaczm85wp1fqazmamdj2bh9wfv3anlslvll606lrfgir"))))
>> +    (build-system cmake-build-system)
>> +    (arguments
>> +     `(#:phases (modify-phases %standard-phases
>
> Please switch to list + gexps.
>
>> +                  (replace 'configure
>> +                    (lambda* (#:key outputs #:allow-other-keys)
>> +                      (invoke "qmake")))
>
> Use plain lambda without arguments (lambda _).
>
>> +                  (add-after 'unpack 'fix-install
>> +                    (lambda* (#:key outputs #:allow-other-keys)
>
> Ditto. 
>> +                      (let* ((out (assoc-ref outputs "out")))
>>
>
> Use gexps variables.
>
>> +                        (substitute* "qtcsv.pro"
>> +                          (("/usr") out)
>> +                          (("\\$\\$PWD") out)))))
>> +                  (replace 'install
>> +                    (lambda* (#:key outputs #:allow-other-keys)
>> +                      (let* ((out (assoc-ref outputs "out")) (lib (string-append
>> +                                                                   out "/lib"))
>
> Ditto :-).
>
>> +                             (include (string-append out "/include")))
>> +                        (mkdir-p include)
>> +                        (mkdir-p lib)
>> +                        (chdir "..") ;we were in dir tests
>
> Same comment bout active tense as earlier... but it seems that'd no
> longer be needed with the next comment implemented?
>
>> +                        (copy-recursively "include" include)
>> +                        (for-each (lambda (file)
>> +                                    (install-file (string-append file) lib))
>> +                                  '("libqtcsv.so" "libqtcsv.so.1"
>> +                                    "libqtcsv.so.1.6" "libqtcsv.so.1.6.0")))))
>> +                  (replace 'check
>> +                    (lambda* (#:key tests? test-options parallel-tests?
>> +                              #:allow-other-keys)
>> +                      (when tests?
>> +                        (chdir "tests")
>> +                        (invoke "qmake")))))))
>
> If qmake has a switch like make to change directory, e.g. -C, that'd be
> preferable, else "with-directory-excursion" to avoid keeping state in
> other phases.
>
> Otherwise, this patch LGTM.

Gently ping :-).  There are also other review comments up-thread.

-- 
Thanks,
Maxim