diff mbox series

[bug#39619,v2] Re: bug#39619: Acknowledgement ([PATCH 0/4] Add nheko matrix client)

Message ID 87zhdbk3ib.fsf@guixSD.i-did-not-set--mail-host-address--so-tickle-me
State Accepted
Headers show
Series [bug#39619,v2] Re: bug#39619: Acknowledgement ([PATCH 0/4] Add nheko matrix client) | expand

Checks

Context Check Description
cbaines/applying patch fail View Laminar job

Commit Message

Nicolò Balzarotti Feb. 21, 2020, 9:57 p.m. UTC 
Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:

Hi, 
> Hello,
>
> Nicolò Balzarotti <anothersms@gmail.com> writes:
>
>> I just noticed that nlohmann-json-cpp is deprecated for json-modern-cxx,
>> fixed it in the two patches that were using it.
>
> Thank you for the patches.
>

thanks for your review.

> Unfortunately, I cannot build nheko because of a missing lmdbxx input.
>
lmdbxx was patch #3, but I created some noise sending replying to
guix-patches and creating multiple issues.  I'm sending it again with
other fixes applied.

> Some comments follow.
>
>> +           (lambda _
>> +             (substitute* "CMakeLists.txt"
>> +               (("add_test\\(BasicConnectivity") "# add_test")
>> +               (("add_test\\(ClientAPI") "# add_test")
>> +               (("add_test\\(MediaAPI") "# add_test")
>> +               (("add_test\\(Encryption") "# add_test"))
>
> Nitpick: I suggest to use a single regexp for these.

Sure, done.  I'm not much confident with substitute* yet.

>
>> +    (inputs
>> +     `(("boost" ,boost)
>> +       ("libolm" ,libolm)
>> +       ("libsodium" ,libsodium)
>> +       ("openssl" ,openssl)
>> +       ("json-modern-cxx" ,json-modern-cxx)
>> +       ("spdlog" ,spdlog)
>> +       ("zlib" ,zlib)))
>
> Could you re-order inputs alphabetically?

Done

>
>> +    (description "@code{mtxclient} is a C++ library that implements client API
>> +for the Matrix protocol.  It's built on to of @code{Boost.Asio}.")
>
> Nitpick: "It's" -> "It is".

Done
>> +    (license license:expat)))
>> +
>>  (define-public quaternion
>>    (package
>>      (name "quaternion")
>> @@ -1795,8 +1849,8 @@ QMatrixClient project.")
>>       (origin
>>         (method git-fetch)
>>         (uri (git-reference
>> -              (url "https://github.com/QMatrixClient/Quaternion")
>> -              (commit version)))
>> +             (url "https://github.com/QMatrixClient/Quaternion")
>> +             (commit version)))
>
> This change is unrelated to the patch. Could you remove it?
I'm sorry
>
>> +    (inputs
>> +     `(("boost" ,boost)
>> +       ("cmark" ,cmark)
>> +       ("libolm" ,libolm)
>> +       ("lmdb" ,lmdb)
>> +       ("lmdbxx" ,lmdbxx)
>
> What is that?
See previous comment

>> +       ("mtxclient" ,mtxclient)
>> +       ("openssl" ,openssl)
>> +       ("json-modern-cxx" ,json-modern-cxx)
>> +       ("qtbase" ,qtbase)
>> +       ("qtsvg" ,qtsvg)
>> +       ("qtmultimedia" ,qtmultimedia)
>> +       ("spdlog" ,spdlog)
>> +       ("tweeny" ,tweeny)
>> +       ("zlib" ,zlib)))
>> +    (native-inputs
>> +     `(("pkg-config" ,pkg-config)
>> +       ("qtlinguist" ,qttools)))
>
> Isn't it a bit confusing?
I copied it from
./gnu/packages/lxqt.scm:1332:          ("qtlinguist" ,qttools)))
./gnu/packages/sync.scm:193:       ("qtlinguist" ,qttools)))
./gnu/packages/music.scm:279:         ("qtlinguist" ,qttools)))
./gnu/packages/music.scm:4128:       ("qtlinguist" ,qttools)))

I can change it if needed, but I found other instances searching for
"qtlinguist" as cmake complained about it.  If those instances were
"qttools" I would have not found it.

>
>> +    (build-system qt-build-system)
>
> Nitpick: usually, build-system is above inputs and arguments.
>
>> +    (home-page "https://github.com/Nheko-Reborn/nheko")
>> +    (synopsis "Desktop client for Matrix using Qt and C++14")
>> +    (description "@code{Nheko} want to provide a native desktop app for the
>> +Matrix protocol that feels more like a mainstream chat app and less like an IRC
>> +client.
>
> "that feels more..." sounds link marketing buzz. Maybe we could remove it.
>
>> +Most of the features you would expect from a chat application are missing right
>> +now but we are getting close to a more feature complete client.
>
> I'm not sure this part is warranted either.
Removed and rephrased

>
>> Specifically
>> +there is support for:
>> +@itemize
>> +@item E2E encryption (text messages only: attachments are currently sent unencrypted).
>> +@item User registration.
>> +@item Creating, joining & leaving rooms.
>> +@item Sending & receiving invites.
>> +@item Sending & receiving files and emoji.
>> +@item Typing notifications.
>> +@item Username auto-completion.
>> +@item Message & mention notifications.
>> +@item Redacting messages.
>> +@item Read receipts.
>> +@item Basic communities support.
>> +@item Room switcher (@key{ctrl-K}).
>> +@item Light, Dark & System themes.
>> +@end itemize\n")
>
> No need for the final newline.

I wasn't sure, as there are dozens of instances with the newline after

rep -ri --line-number  '@end itemize[^\\n]'  | wc -l
181

grep -ri --line-number  '@end itemize\\n'  | wc -l
277


I removed it, but if there's a standard, it would be better if it was
respected (as for "new" contributors it's difficult to understand the
right way just by looking at existing packages)
Thanks, Nicolò


>
> Regards,
>
> -- 
> Nicolas Goaziou

Comments

Nicolas Goaziou Feb. 22, 2020, 11:48 p.m. UTC | #1 
Hello,

Nicolò Balzarotti <anothersms@gmail.com> writes:

> lmdbxx was patch #3, but I created some noise sending replying to
> guix-patches and creating multiple issues.  I'm sending it again with
> other fixes applied.

OK. I applied your patches with the two small changes below:
> +         (add-before 'configure 'disable-network-tests
> +           (lambda _
> +             (substitute* "CMakeLists.txt"
> +               (("add_test\\([BasicConnectivity|ClientAPI|MediaAPI|Encryption]")
> +                "# add_test"))

I changed the regexp to:

  "add_test\\((BasicConnectivity|ClientAPI|MediaAPI|Encryption)"

instead.

> +    (source (origin
> +              (method git-fetch)
> +              (uri (git-reference
> +                    (url "https://github.com/drycpp/lmdbxx.git")
> +                    (commit (string-append version))))

I removed the `string-append' call.

I also added copyright lines for you in "cpp.scm" and "databases.scm".

Thank you!

Regards,
diff mbox series

Patch

From 38ac0d6fe76c2202118dd6499110974062ff7232 Mon Sep 17 00:00:00 2001
From: nixo <nicolo@nixo.xyz>
Date: Sat, 15 Feb 2020 21:08:12 +0100
Subject: [PATCH 4/4] gnu: Add nheko.

* gnu/packages/messaging.scm (nheko): New variable.
---
 gnu/packages/messaging.scm | 76 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 76 insertions(+)

diff --git a/gnu/packages/messaging.scm b/gnu/packages/messaging.scm
index df80168d7c..9e665fb08c 100644
--- a/gnu/packages/messaging.scm
+++ b/gnu/packages/messaging.scm
@@ -18,6 +18,7 @@ 
 ;;; Copyright © 2019 Tanguy Le Carrour <tanguy@bioneland.org>
 ;;; Copyright © 2019, 2020 Brett Gilio <brettg@gnu.org>
 ;;; Copyright © 2019, 2020 Timotej Lazar <timotej.lazar@araneo.si>
+;;; Copyright © 2020 Nicolò Balzarotti <nicolo@nixo.xyz>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -72,6 +73,7 @@ 
   #:use-module (gnu packages logging)
   #:use-module (gnu packages lua)
   #:use-module (gnu packages man)
+  #:use-module (gnu packages markup)
   #:use-module (gnu packages ncurses)
   #:use-module (gnu packages networking)
   #:use-module (gnu packages pcre)
@@ -1839,6 +1841,80 @@  QMatrixClient project.")
 for the Matrix protocol.  It is built on to of @code{Boost.Asio}.")
     (license license:expat)))
 
+(define-public nheko
+  (package
+    (name "nheko")
+    (version "0.6.4")
+    (source (origin
+              (method git-fetch)
+              (uri (git-reference
+                    (url "https://github.com/Nheko-Reborn/nheko.git")
+                    (commit (string-append "v" version))))
+              (file-name (git-file-name name version))
+              (sha256
+               (base32
+                "19dkc98l1q4070v6mli4ybqn0ip0za607w39hjf0x8rqdxq45iwm"))))
+    (arguments
+     `(#:tests? #f                      ; no test target
+       #:configure-flags
+       (list
+        "-DCMAKE_BUILD_TYPE=Release"
+        "-DCMAKE_CXX_FLAGS=-fpermissive")
+       #:phases
+       (modify-phases %standard-phases
+         (add-after 'unpack 'remove-Werror
+           (lambda _
+             (substitute* "CMakeLists.txt"
+               (("-Werror") ""))
+             #t))
+         (add-after 'unpack 'fix-determinism
+           (lambda _
+             ;; Make Qt deterministic.
+             (setenv "QT_RCC_SOURCE_DATE_OVERRIDE" "1")
+             #t)))))
+    (build-system qt-build-system)
+    (inputs
+     `(("boost" ,boost)
+       ("cmark" ,cmark)
+       ("json-modern-cxx" ,json-modern-cxx)
+       ("libolm" ,libolm)
+       ("lmdb" ,lmdb)
+       ("lmdbxx" ,lmdbxx)
+       ("mtxclient" ,mtxclient)
+       ("openssl" ,openssl)
+       ("qtbase" ,qtbase)
+       ("qtsvg" ,qtsvg)
+       ("qtmultimedia" ,qtmultimedia)
+       ("spdlog" ,spdlog)
+       ("tweeny" ,tweeny)
+       ("zlib" ,zlib)))
+    (native-inputs
+     `(("pkg-config" ,pkg-config)
+       ("qtlinguist" ,qttools)))
+    (home-page "https://github.com/Nheko-Reborn/nheko")
+    (synopsis "Desktop client for Matrix using Qt and C++14")
+    (description "@code{Nheko} want to provide a native desktop app for the
+Matrix protocol that feels more like a mainstream chat app and less like an IRC
+client.
+
+There is support for:
+@itemize
+@item E2E encryption (text messages only: attachments are currently sent unencrypted).
+@item User registration.
+@item Creating, joining & leaving rooms.
+@item Sending & receiving invites.
+@item Sending & receiving files and emoji.
+@item Typing notifications.
+@item Username auto-completion.
+@item Message & mention notifications.
+@item Redacting messages.
+@item Read receipts.
+@item Basic communities support.
+@item Room switcher (@key{ctrl-K}).
+@item Light, Dark & System themes.
+@end itemize")
+    (license license:gpl3+)))
+
 (define-public quaternion
   (package
     (name "quaternion")
-- 
2.25.0