GNU bug report logs - #45889
Nextcloud Client

Previous Next

Full log

Message #17 received at 45889 <at> debbugs.gnu.org (full text, mbox):

From: Nicolò Balzarotti <anothersms <at> gmail.com>
To: Raghav Gururajan <rg <at> raghavgururajan.name>, 45889 <at> debbugs.gnu.org
Subject: Re: [bug#45889] Nextcloud Desktop (v4)
Date: Sat, 16 Jan 2021 11:48:16 +0100

Raghav Gururajan <rg <at> 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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.