unofficial mirror of guix-patches@gnu.org 
 help / color / mirror / code / Atom feed
From: Leo Prikler <leo.prikler@student.tugraz.at>
To: 45721@debbugs.gnu.org
Cc: Raghav Gururajan <rg@raghavgururajan.name>
Subject: [bug#45721] Telegram Desktop (v9)
Date: Sat, 16 Jan 2021 19:04:35 +0100	[thread overview]
Message-ID: <260455952500fcfd067f395a55a7ac78a4f1bd0a.camel@student.tugraz.at> (raw)
In-Reply-To: <b2339e11-707e-7f7b-9e74-ce4196c8c0f1@raghavgururajan.name>

Hi Raghav,

congratulations on getting a working Telegram Desktop.  I haven't yet
built this version on my own, but I want to comment on the patches a
little.

Am Freitag, den 15.01.2021, 11:08 -0500 schrieb Raghav Gururajan:
> * gnu/packages/cpp.scm (gsl): New variable.
> * gnu/packages/patches/gsl-gtest.patch: New file.
> * gnu/local.mk (dist_patch_DATA): Add it.
LGTM, but there seem to be whitespace issues.  Any idea?

>  (define-module (gnu packages fcitx)
> -  #:use-module ((guix licenses) #:select (gpl2+))
> +  #:use-module (guix licenses)
Personal nitpick, but those should likely be prefixed with license:
To keep backwards-compatibility, you can (define gpl2+ license:gpl2+)
inside the module.

> * gnu/packages/language.scm (libchewing): New variable.
LGTM.

> +         (add-after 'unpack 'patch-std
> +           (lambda _
> +             (substitute* '("configure" "Makefile")
> +               (("17")
> +                "11"))
> +             #t))
Is there a less broad way of doing this?  E.g. replacing c++-17 by c++-
11?

> +        (git-reference
> +         (url "https://github.com/hamonikr/nimf.git")
> +         (commit
> +          (string-append "nimf-" version))))
FWIW there seems to also exist an older version over at
https://gitlab.com/nimf-i18n/nimf
Would it be worth packaging that?

> +    (synopsis "Korean input method framework")
> +    (description "Nimf is a lightweight, fast and extensible input
> method
> +framework.")
In my opinion the synopsis should be "Lightweight input method
framework" and the description should mention, that this specific fork
has a special focus on Korean.

> * gnu/packages/cmake.scm (cmake-shared): New variable.
LGTM, albeit admittedly weird.

> +    (synopsis "Material Decoration for Qt")
> +    (description "MaterialDecoration is the client-side decoration
> for Qt
> +applications on Wayland.")
It's not "the", just "a".  Usually such projects describe Material
Design in some way, but apparently it has come to a point where just
throwing around the word "Material" is enough.

> +    (description "Range-v3 is the range library for
> C++14/17/20.  This code was
> +the basis of a formal proposal to add range support to the C++
> standard library.")
I find the following more useful:
> [range-v3 is] an extension of the Standard Template Library that
> makes its iterators and algorithms more powerful by making them
> composable.  Unlike other range-like solutions which, seek to do away
> with iterators, in range-v3 ranges are an abstration layer on top of
> iterators.

> +    (license
> +     (list
> +      ;; Dual-Licensed
> +      license:expat
> +      license:ncsa))))
It appears, that this library carries a few more (free) licenses with
it.  Perhaps this needs to be revised?

> * gnu/packages/cpp.scm (rlottie): New variable.
LGTM.

> * gnu/packages/qt.scm (qt5ct): New variable.
LGTM.

> +(define-public tg_owt
> +  (package
> +    (name "tg_owt")
> +    (version "0.0.0")
Use a proper version.  Packages, that build directly from git without
any tagged versions usually have a preamble of 
  (let ((commit <hash>) 
        (revision <number))
    (package ...))

> +    (source
> +     (origin
> +       (method git-fetch)
> +       (uri
> +        (git-reference
> +         (url "https://github.com/desktop-app/tg_owt.git")
> +         (commit "fa86fcc")
> +         (recursive? #t)))
Is there a way of making this checkout non-recursive?  I know you've
made that change in accordance to an upstream recommendation, but one
ought to look at it a little closer. 

> +    (inputs
> +     `(("abseil-cpp" ,abseil-cpp)
> +       ("alsa" ,alsa-lib)
> +       ("ffmpeg" ,ffmpeg)
> +       ("libjpeg" ,libjpeg-turbo)
> +       ("libsrtp" ,libsrtp)
> +       ("libvpx" ,libvpx)
> +       ;; ("libyuv" ,libyuv)
> +       ("openh264" ,openh264)
> +       ("openssl" ,openssl)
> +       ("opus" ,opus)
> +       ("protobuf" ,protobuf)
> +       ("pulseaudio" ,pulseaudio)
> +       ("rnnoise" ,rnnoise)))
It seems that some of those inputs are also found as third_party/
libraries.  Can you remove their respective sources from the package?

> +    (synopsis "WebRTC build for Telegram")
> +    (description "TG_OWT is the packaged build of WebRTC, for its
> use in
> +Telegram-Desktop application.")
I really don't like that synopsis and description.  Granted, upstream
offers little to work with, but there ought to be a better way of
phrasing this.

By the way, it would appear we already have some WebRTC stuff packaged,
but no direct "webrtc" package (which I guess is normal, given that it
is a protocol and not an individual piece of software).  How tightly is
Telegram coupled to this specific implementation?  Would there be a way
of replacing it with something else (kinda like our udev/eudev
situation)?

> * gnu/packages/telegram.scm: New module.
> * gnu/local.mk (GNU_SYSTEM_MODULES): Add it.
> * gnu/packages/telegram.scm (tdesktop): New variable.
Would there be a way of moving this into another module, e.g. (gnu
packages messaging)?

Regards,
Leo





  parent reply	other threads:[~2021-01-16 18:05 UTC|newest]

Thread overview: 55+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-01-08  0:20 [bug#45721] Telegram Desktop Raghav Gururajan
2021-01-08  0:44 ` [bug#45721] Telegram Desktop (v2) Raghav Gururajan
2021-01-12  3:12 ` [bug#45721] Telegram Desktop (v3) Raghav Gururajan
2021-01-12  3:27 ` [bug#45721] Telegram Desktop (v4) Raghav Gururajan
2021-01-13 22:13 ` [bug#45721] Telegram Desktop (v5) Raghav Gururajan
2021-01-14  5:39 ` [bug#45721] Telegram Desktop (v6) Raghav Gururajan
2021-01-14  6:41 ` [bug#45721] Telegram Desktop (v7) Raghav Gururajan
2021-01-14 10:38 ` [bug#45721] Telegram Desktop (v8) Raghav Gururajan
2021-01-16 16:08 ` [bug#45721] Telegram Desktop (v9) Raghav Gururajan
2021-01-16 17:15   ` Nicolò Balzarotti
2021-01-16 21:22     ` Raghav Gururajan
2021-01-16 18:04 ` Leo Prikler [this message]
2021-01-17  0:19   ` Raghav Gururajan
2021-01-17  0:36     ` Leo Prikler
2021-01-17  1:10       ` Raghav Gururajan
2021-01-17  0:05 ` [bug#45721] Telegram Desktop (v10) Raghav Gururajan
2021-01-17  0:29 ` [bug#45721] Telegram Desktop (v11) Raghav Gururajan
2021-01-17  1:04 ` [bug#45721] Telegram Desktop (v12) Raghav Gururajan
2021-01-17  1:52 ` [bug#45721] Telegram Desktop (v13) Raghav Gururajan
2021-01-17 12:13   ` Leo Prikler
2021-01-17 14:49     ` Raghav Gururajan
2021-01-17 19:08       ` [bug#45721] [PATCH v15] Add Telegram Desktop Leo Prikler
2021-01-17 21:59         ` Raghav Gururajan
2021-01-19 10:11         ` [bug#45721] [PATCH v16] " Leo Prikler
2021-01-17 14:43 ` [bug#45721] Telegram Desktop (v14) Raghav Gururajan
2021-01-20 10:41 ` [bug#45721] Telegram Desktop (v17) Raghav Gururajan
2021-01-20 11:07   ` Leo Prikler
2021-01-20 12:32     ` Raghav Gururajan
2021-01-20 12:29 ` [bug#45721] Telegram Desktop (v18) Raghav Gururajan
2021-01-20 13:49 ` [bug#45721] Telegram Desktop (v19) Raghav Gururajan
2021-01-20 14:42 ` [bug#45721] Telegram Desktop (v20) Raghav Gururajan
2021-01-20 15:10   ` Leo Prikler
2021-01-21  3:44 ` [bug#45721] Telegram Desktop (v21) Raghav Gururajan
2021-01-21 19:11   ` [bug#45721] [PATCH v22] Add Telegram Desktop Leo Prikler
2021-01-21 19:37     ` Raghav Gururajan
2021-01-21 19:44       ` Leo Prikler
2021-01-21 19:46         ` Raghav Gururajan
2021-01-22  4:27 ` [bug#45721] Telegram Desktop (v23) Raghav Gururajan
2021-01-22  7:42   ` Leo Prikler
2021-01-28  0:41     ` Raghav Gururajan
2021-01-28  0:41 ` [bug#45721] Telegram Desktop (v24) Raghav Gururajan
2021-01-28  7:47   ` Leo Prikler
2021-01-30 17:04 ` [bug#45721] Telegram Desktop (v25) Raghav Gururajan
     [not found]   ` <e881733992b22e238f2b90149dbceac6c3467180.camel@student.tugraz.at>
     [not found]     ` <e5040a67-824f-17c8-e16f-1a68531b3d5f@raghavgururajan.name>
2021-01-30 18:24       ` Leo Prikler
2021-01-30 19:30         ` Raghav Gururajan
2021-01-30 18:02 ` [bug#45721] Telegram Desktop (v26) Raghav Gururajan
2021-01-30 19:02 ` [bug#45721] Telegram Desktop (v27) Raghav Gururajan
2021-01-31  8:37   ` bug#45721: " Leo Prikler
2021-01-31 19:11     ` [bug#45721] " Raghav Gururajan
2021-01-31 13:57   ` Jonathan Brielmaier
2021-01-31 14:10     ` Leo Prikler
2021-01-31 16:50       ` Jonathan Brielmaier
2021-01-31 16:56         ` Leo Prikler
2021-01-31 17:01           ` Jonathan Brielmaier
2021-01-31 19:14             ` Raghav Gururajan

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://guix.gnu.org/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=260455952500fcfd067f395a55a7ac78a4f1bd0a.camel@student.tugraz.at \
    --to=leo.prikler@student.tugraz.at \
    --cc=45721@debbugs.gnu.org \
    --cc=rg@raghavgururajan.name \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/guix.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).