[bug#45889] Nextcloud Desktop (v4)

["Nicolò Balzarotti" <anothersms@gmail.com>]

Raghav Gururajan <rg@raghavgururajan.name> writes:

Hi!
> Please find the attached patch-set to add Nextcloud desktop application 
> to guix.
I usually don't do patch review, so don't weight too much my comments.

Why is it placed inside a new module?  It should fit nicely in sync.scm

> +         (url "https://github.com/nextcloud/desktop.git")
.git shouldn't be needed

> +        (base32 "1ba9z1kv3wlrmaxsn442vn0inzbd0smvq4xkavarn1h8i0dm62hb"))))
This hash is wrong, I get 022k7b3c30dymrjc1g3ly2cac1c34gkqnvjya6p7w2j3qw2w1dm2

> +             (with-directory-excursion "src/3rdparty"
> +               (for-each delete-file-recursively
> +                         (list
> +                          "libcrashreporter-qt"
> +                          "sqlite3")))
This can be expressed in terms of what you you are keeping instead.

Something along the line of:

(let ((preserved-3rdparty-files
                '("QProgressIndicator" "qtlockedfile" "qtokenizer"
                  "qtsingleapplication" "kmessagewidget")))
           (with-directory-excursion "src/3rdparty"
             (for-each
              (lambda (directory)
                (simple-format #t "deleting: ~A\n" directory)
                (delete-file-recursively directory))
              (lset-difference string=?
                               (scandir ".")
                               (cons* "." ".." preserved-3rdparty-files))))
           #t)

> +         (add-after 'remove-thirdparty 'patch-plugin-dirs
> +           (lambda* (#:key outputs #:allow-other-keys)
> +             (substitute* "shell_integration/libcloudproviders/CMakeLists.txt"
> +               (("PKGCONFIG_GETVAR\\(dbus-1 session_bus_services_dir
> _install_dir\\)")
This line is too long, you can use something like
("PKGCONFIG_GETVAR\\(.*") instead.

> +                "set(_install_dir> \"${CMAKE_INSTALL_PREFIX}/share/dbus-1/services\")"))
Other long line, maybe string-append.

> +             (substitute* "shell_integration/dolphin/CMakeLists.txt"
> +               (("ON CACHE")
> +                "OFF CACHE"))
> +             #t))

Why?

> +         (add-before 'check 'pre-check
> +           (lambda _
> +             (setenv "HOME" (getcwd))
> +             #t))

It's missing a comment on why this is needed (like qttest tries to create
$HOME/.qttest/config/autostart/)
> +    (license license:gpl2+)))

Unbundled dependencies have different licenses
QprogressIndicator is under expat while others lgpl2.1+ if I'm not wrong

Also, I tried removing ruby and python-sphinx from the dependencies and
it did build fine, so be sure all of them are needed (or are they used
for some optional feature?).

Thanks