GNU bug report logs - #42357
[PATCH] add package definition for Nextcloud Desktop

Previous Next

Full log

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

From: Tobias Geerinckx-Rice <me <at> tobias.gr>
To: Tim Magee <timothy <at> eastlincoln.net>
Cc: 42357 <at> debbugs.gnu.org
Subject: Re: [bug#42357] [PATCH] add package definition for Nextcloud Desktop
Date: Wed, 15 Jul 2020 01:32:10 +0200

[Message part 1 (text/plain, inline)]

Tim,

Thank you!  I hope you enjoyed making your first submitted Guix 
package.

Some things that need to be taken care of before it can be merged:

Tim Magee 写道：
> +  %D%/packages/patches/nextxcloud-fix-filenames.patch 
> \

Sneaky typo.  ./pre-inst-env won't care but it would break ‘guix 
pull’.

> --- /dev/null
> +++ b/gnu/packages/nextcloud.scm
> @@ -0,0 +1,397 @@
> +;;; GNU Guix --- Functional package management for GNU
> +;;; Copyright © 2015, 2016, 2017, 2018, 2019 Efraim Flashner
> <efraim <at> flashner.co.il>
> +;;; Copyright © 2017 Ricardo Wurmus <rekado <at> elephly.net>
> +;;; Copyright © 2018, 2019, 2020 Tobias Geerinckx-Rice 
> <me <at> tobias.gr>
> +;;; Copyright © 2018 Ludovic Courtès <ludo <at> gnu.org>
> +;;; Copyright © 2018, 2019, 2020 Nicolas Goaziou 
> <mail <at> nicolasgoaziou.fr>
> +;;; Copyright © 2019 Clément Lassieur <clement <at> lassieur.org>
> +;;; Copyright © 2020 Jakub Kądziołka <kuba <at> kadziolka.net>
> +;;; Copyright © 2020 Tim Magee <timothy <at> eastlincoln.net>

Humm… oh.

I guess you first added NextCloud to sync.scm, then decided to put 
it in its own file (or maybe because I told you so :-).  Don't 
forget to delete all the previous copyright lines (unless you 
borrowed code from elsewhere), and packages other than 
nextcloud-desktop.

> +(define-module (gnu packages nextcloud)
> +  #:use-module ((guix licenses) #:prefix license:)
[…]

Best to delete this list, start from scratch, copy back only 
what's needed to build nextcloud-desktop.

> +(define-public nextcloud-desktop
> +  (package
> +    (name "nextcloud-desktop")
> +    (version "2.6.5")
> +    (source
> +     (origin
> +       (method url-fetch)
> +       (uri (string-append "https://github.com/"
> +                           "nextcloud/desktop/archive/v" 
> version
> ".tar.gz"))

Avoid GitHub's /archive/ tarballs.  They're auto-generated and 
provide no value over a git checkout.  They can also change 
unexpectedly leading to hash mismatches.  This has happened in the 
past.

Instead:

--8<---------------cut here---------------start------------->8---
 (source
  (origin
    (method git-fetch)
    (uri (git-reference
          (url "https://github.com/nextcloud/desktop")
          (commit (string-append "v" version))))
    (sha256
     (base32 "…"))
    (file-name (git-file-name name version))))
--8<---------------cut here---------------end--------------->8---

COMMIT can actually be any supported git ref; here it's a tag 
which makes updating the package a breeze.

> +       ;; I cherry picked this patch from Nextcloud
> +       (patches (search-patches 
> "nextcloud-fix-filenames.patch"))

Please move the comment to the patch file (you can type whatever 
you want above its first line) and include a link to the exact 
upstream commit.

The patch file itself is missing from this patch, you probably 
need to ‘git add’ it, then ‘git commit --amend’.

> +       (modules '((guix build utils)))
> +       (snippet
> +        '(begin
> +           ;; libcrashreporter-qt has its own bundled 
> dependencies
> +           (delete-file-recursively 
> "src/3rdparty/libcrashreporter-qt")
> +           (delete-file-recursively "src/3rdparty/sqlite3")
> +           ;; qprogessindicator, qlockedfile, qtokenizer and
> +           ;; qtsingleapplication have not yet been packaged, 
> but all are
> +           ;; explicitly used from the 3rdparty folder during 
> build.
> +           #t))))
> +    (build-system cmake-build-system)
> +    (arguments
> +     `(#:phases
> +       (modify-phases %standard-phases
> +	 (add-after 'unpack 'delete-failing-utility-test
  ^^^^^^

This is a tab, which is character non grata in Guix Scheme code. 
I won't point them all out: ‘guix lint’ should warn you about 
them.

If you're not using emacs, check out etc/indent-code.el in the 
Guix source tree.  It will indent your code for you using emacs 
and Guix's style rules.

> + 	   ;; "Could not create autostart folder"

Nitpick: ;;-comments are sentences, hence capitalised & ending 
with a full stop.  Same for the snippet comments above, but libfoo 
can remain lowercase there.

> +	   (lambda _
> +	     (substitute* "test/CMakeLists.txt"
> +			  (("nextcloud_add_test\\(Utility \"\"\\)" 
> test)
> +			   (string-append "#" test)))
> +	     #t))
> +	 (add-after 'unpack 'delete-failing-files-test

These 2 can be combined into one ‘skip-failing-tests’ phase, and 
even one single substitute* call:

--8<---------------cut here---------------start------------->8---
 (substitute* "test/CMakeLists.txt"
  (("nextcloud_add_test\\(Utility \"\"\\)" test)
   (string-append "#" test))
  (("nextcloud_add_test\\(AllFilesDeleted 
  \"syncenginetestutils.h\"\\)" test)
   (string-append "#" test)))
--8<---------------cut here---------------end--------------->8---

> +         (delete 'patch-dot-desktop-files))

Please explain why in a comment.

> +       #:configure-flags '("-DUNIT_TESTING=ON")))

Thanks for taking the trouble to enable and fix tests!

> +    (native-inputs
> +     `(("cmocka" ,cmocka)
> +       ("perl" ,perl)
> +       ("pkg-config" ,pkg-config)
> +       ("qtlinguist" ,qttools)))
> +    (inputs
> +     `(("openssl" , openssl)
> +       ("qtbase" ,qtbase)
> +       ("qtdeclarative" ,qtdeclarative)
> +       ("qtkeychain" ,qtkeychain)
> +;;       ("qtquickcontrols" ,qtquickcontrols)
> +;;       ("qtquickcontrols2" ,qtquickcontrols2)

What's the deal with these 2?  If you want to keep them as a 
warning/promise to others, please elaborate in a comment.

> +       ("qtwebchannel", qtwebchannel)
> +       ("qtwebengine" ,qtwebengine)
> +       ("qtwebkit" ,qtwebkit)
> +       ("sqlite" ,sqlite)
> +       ("zlib" ,zlib)))
> +    (home-page "https://nextcloud.org")
> +    (synopsis "Folder synchronization with a Nextcloud server")

s/folder/file/?  Or does it synchronise more than files and 
directories?

> +    (description "Use the Nextcloud desktop client to keep your 
> files
> synchronized between your Nextcloud server and your 
> desktop. Select one

We always double-space sentences in Texinfo (like.  this.).  ‘guix 
lint’ should warn you of this.

> or more directories on your local machine and always have access 
> to your
> latest files wherever you are.")
> +    (license license:gpl2+)))

I'm about to fall asleep and will call it a day.  I'll actually 
build the package and double-check the licence tomorrow, if nobody 
beats me to it.  Don't hesitate to holler if I forget.

Thanks again for sharing this with us!

Kind regards,

T G-R

[signature.asc (application/pgp-signature, inline)]

Previous Next

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