[bug#45721] Telegram Desktop

Hello Guix!

This patch-series is for adding Telegram-Desktop application and its 
dependencies to Guix.

Regards,
RG.

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

Hi Leo!

> 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.

Thanks! Couldn't have done without your help. :-)

> LGTM, but there seem to be whitespace issues.  Any idea?

Those are from the patch file taken from upstream. The pack-def is clean.

> Personal nitpick, but those should likely be prefixed with license:
> To keep backwards-compatibility, you can (define gpl2+ license:gpl2+)
> inside the module.

Hmm. I think we have to make changes to all pack-def in this module. As 
the licenses doesn't use prefix. Can be a separate task.

> LGTM.

Cool!

> Is there a less broad way of doing this?  E.g. replacing c++-17 by c++-
> 11?

Updated in v10.

> FWIW there seems to also exist an older version over at
> https://gitlab.com/nimf-i18n/nimf
> Would it be worth packaging that?

Hmm, not sure. I can package it later though.

> 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.

Updated in v10.

> LGTM, albeit admittedly weird.

Haha, yeah.

> 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.

Updated in v10.

> 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.

Updated in v10.

> It appears, that this library carries a few more (free) licenses with
> it.  Perhaps this needs to be revised?

Updated in v10.

> LGTM.

Cool!

> LGTM.

Cool!

> 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 ...))

Updated in v10.

> 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.

Not sure, but we need the sub-modules any way for build.

> It seems that some of those inputs are also found as third_party/
> libraries.  Can you remove their respective sources from the package?

Updated in v10.

> 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.

Updated in v10.

> 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)?

Nah, telegram made a hard fork. There are some telegram-specific classes 
and objects.

> Would there be a way of moving this into another module, e.g. (gnu
> packages messaging)?

I first tried there, but the was circular dependency. So moved it to 
separate module. We can also move telegram-related stuff like tg_owt 
etc, to this module in the future.

Thank you so much for reviewing. :-)

Regards,
RG.

Hi Raghav,

Am Samstag, den 16.01.2021, 19:19 -0500 schrieb Raghav Gururajan:
> Hi Leo!
> 
> > 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.
> 
> Thanks! Couldn't have done without your help. :-)
> 
> > LGTM, but there seem to be whitespace issues.  Any idea?
> 
> Those are from the patch file taken from upstream. The pack-def is
> clean.
Nvm then.

> > Personal nitpick, but those should likely be prefixed with license:
> > To keep backwards-compatibility, you can (define gpl2+
> > license:gpl2+)
> > inside the module.
> 
> Hmm. I think we have to make changes to all pack-def in this module.
> As 
> the licenses doesn't use prefix. Can be a separate task.
Again, you can (define gpl2+ license:gpl2+) at the start of the module,
so that existing definitions can be kept the same, but your packages
adhere to the license: style.

> > LGTM.
> 
> Cool!
> 
> > Is there a less broad way of doing this?  E.g. replacing c++-17 by
> > c++-
> > 11?
> 
> Updated in v10.
Yes, okay.

> > 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.
> 
> Updated in v10.
Already discussed that one in IRC.

> > LGTM, albeit admittedly weird.
> 
> Haha, yeah.
> 
> > 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.
> 
> Updated in v10.
Okay.

> > 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.
> 
> Updated in v10.
Thanks.

> > It appears, that this library carries a few more (free) licenses
> > with
> > it.  Perhaps this needs to be revised?
> 
> Updated in v10.
Seems okay, I also saw you double-checking in IRC.

> > 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 ...))
> 
> Updated in v10.
Use full commit hashes please.

> > 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.
> 
> Not sure, but we need the sub-modules any way for build.
Perhaps, but it seems kinda weird, that this package gets special
treatment in how we accept anything else it might pull in.

> > It seems that some of those inputs are also found as third_party/
> > libraries.  Can you remove their respective sources from the
> > package?
> 
> Updated in v10.
I don't particularly agree with the way this has been solved in v10. 
Do we really need to keep around custom forks and old versions?  Can we
not instead patch tg_owt?

> > 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.
> 
> Updated in v10.
Yep, that sounds better.

> > 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)?
> 
> Nah, telegram made a hard fork. There are some telegram-specific
> classes 
> and objects.
Fair enough.

> > Would there be a way of moving this into another module, e.g. (gnu
> > packages messaging)?
> 
> I first tried there, but the was circular dependency. So moved it to 
> separate module. We can also move telegram-related stuff like tg_owt 
> etc, to this module in the future.
That would make sense in my opinion.

Regards,
Leo

Hi Leo!

> Again, you can (define gpl2+ license:gpl2+) at the start of the module,
> so that existing definitions can be kept the same, but your packages
> adhere to the license: style.

Updated in v12, as per discussed in IRC.

> Use full commit hashes please.

Updated in v12.

> Perhaps, but it seems kinda weird, that this package gets special
> treatment in how we accept anything else it might pull in.

There are existing packages in guix that uses recursive. But your 
concern is valid. May be we can update git-fetch in guix to have an 
option for fetching only specific/mentioned sub-modules.

> I don't particularly agree with the way this has been solved in v10.
> Do we really need to keep around custom forks and old versions?  Can we
> not instead patch tg_owt?

The situation is same as with tg_owt. They made custom changes in the 
fork. Like unique foo::bar.

Regards,
RG.