Subject: Re: bug#58341: [PATCH] Add x11vnc recipe

From: Mehmet Tekman <[email-masked]>
Date: Mon, 10 Oct 2022 18:05:49 +0200

Hello, thank you for the review!

I've put the recipe into vnc.scm and built to a newer commit that
makes the patches redundant (thanks for the tip, I was basing my
recipe off one from another distro).

I've fixed the commit message and linted and styled the vnc.scm file.

I created a new branch and put my changes there, so please disregard
the above changes.

I attach here the patch file, as I don't quite trust myself to paste
the changes into this email.

Looking forward to the next review,

Best,
Mehmet

On Sat, 8 Oct 2022 at 06:33, Maxim Cournoyer <[email-masked]> wrote:
>
> Hello, and thank you for your contribution!
>
> Mehmet Tekman <[email-masked]> writes:
>
> > ---
> >  gnu/packages/patches/x11vnc-gcc10-fix.patch   | 42 ++++++++++
> >  ...c-scan-limit-access-to-shared-memory.patch | 22 ++++++
> >  .../patches/x11vnc-xfc-null-ptr.patch         | 29 +++++++
> >  gnu/packages/x11vnc.scm                       | 79 +++++++++++++++++++
> >  4 files changed, 172 insertions(+)
> >  create mode 100644 gnu/packages/patches/x11vnc-gcc10-fix.patch
> >  create mode 100644
> > gnu/packages/patches/x11vnc-scan-limit-access-to-shared-memory.patch
> >  create mode 100644 gnu/packages/patches/x11vnc-xfc-null-ptr.patch
> >  create mode 100644 gnu/packages/x11vnc.scm
>
> Would it be possible to send the patch complete with the commit message,
> as can be produced with 'git format-patch'?  You can read 'info "(guix)
> Submitting Patches".  The git commit message should use the GNU
> ChangeLog format, as mentioned there.  You can look at the git history
> to learn.
>
> The patches should be registered in the gnu/local.mk file, in the
> dist_patch_DATA section.  Please mind the sorting in the file.
>
> > diff --git a/gnu/packages/patches/x11vnc-gcc10-fix.patch
> > b/gnu/packages/patches/x11vnc-gcc10-fix.patch
> > new file mode 100644
> > index 0000000000..a32d5785d9
> > --- /dev/null
> > +++ b/gnu/packages/patches/x11vnc-gcc10-fix.patch
> > @@ -0,0 +1,42 @@
> > +From a48b0b1cd887d7f3ae67f525d7d334bd2feffe60 Mon Sep 17 00:00:00 2001
> > +From: Alexander Tsoy <[email-masked]>
> > +Date: Tue, 28 Jan 2020 22:21:01 +0300
> > +Subject: [PATCH] Fix build with -fno-common
> > +
> > +GCC 10 defaults to -fno-common
> > +---
> > + src/util.c | 3 +++
> > + src/util.h | 6 +++---
> > + 2 files changed, 6 insertions(+), 3 deletions(-)
> > +
> > +diff --git a/src/util.c b/src/util.c
> > +index a82a1a42..6a52ebf4 100644
> > +--- a/src/util.c
> > ++++ b/src/util.c
> > +@@ -47,6 +47,9 @@ int hxl = 0;
> > + #ifdef LIBVNCSERVER_HAVE_LIBPTHREAD
> > + MUTEX(x11Mutex);
> > + MUTEX(scrollMutex);
> > ++MUTEX(clientMutex);
> > ++MUTEX(inputMutex);
> > ++MUTEX(pointerMutex);
> > + #endif
> > +
> > + int nfix(int i, int n);
> > +diff --git a/src/util.h b/src/util.h
> > +index 35c1afd2..99b5dd1d 100644
> > +--- a/src/util.h
> > ++++ b/src/util.h
> > +@@ -102,9 +102,9 @@ extern struct timeval _mysleep;
> > + #ifdef LIBVNCSERVER_HAVE_LIBPTHREAD
> > + extern MUTEX(x11Mutex);
> > + extern MUTEX(scrollMutex);
> > +-MUTEX(clientMutex);
> > +-MUTEX(inputMutex);
> > +-MUTEX(pointerMutex);
> > ++extern MUTEX(clientMutex);
> > ++extern MUTEX(inputMutex);
> > ++extern MUTEX(pointerMutex);
> > + #endif
> > +
> > + #define X_INIT INIT_MUTEX(x11Mutex)
> > diff --git a/gnu/packages/patches/x11vnc-scan-limit-access-to-shared-memory.patch
> > b/gnu/packages/patches/x11vnc-scan-limit-access-to-shared-memory.patch
> > new file mode 100644
> > index 0000000000..5424094434
> > --- /dev/null
> > +++ b/gnu/packages/patches/x11vnc-scan-limit-access-to-shared-memory.patch
> > @@ -0,0 +1,22 @@
> > +From 69eeb9f7baa14ca03b16c9de821f9876def7a36a Mon Sep 17 00:00:00 2001
> > +From: =?UTF-8?q?Gu=C3=A9nal=20DAVALAN?= <[email-masked]>
> > +Date: Wed, 18 Nov 2020 08:40:45 +0100
> > +Subject: [PATCH] scan: limit access to shared memory segments to current user
> > +
> > +---
> > + src/scan.c | 2 +-
> > + 1 file changed, 1 insertion(+), 1 deletion(-)
> > +
> > +diff --git a/src/scan.c b/src/scan.c
> > +index 43e00d20..12994d52 100644
> > +--- a/src/scan.c
> > ++++ b/src/scan.c
> > +@@ -320,7 +320,7 @@ static int shm_create(XShmSegmentInfo *shm,
> > XImage **ximg_ptr, int w, int h,
> > +
> > + #if HAVE_XSHM
> > +     shm->shmid = shmget(IPC_PRIVATE,
> > +-        xim->bytes_per_line * xim->height, IPC_CREAT | 0777);
> > ++        xim->bytes_per_line * xim->height, IPC_CREAT | 0600);
> > +
> > +     if (shm->shmid == -1) {
> > +         rfbErr("shmget(%s) failed.\n", name);
> > diff --git a/gnu/packages/patches/x11vnc-xfc-null-ptr.patch
> > b/gnu/packages/patches/x11vnc-xfc-null-ptr.patch
> > new file mode 100644
> > index 0000000000..65f339d716
> > --- /dev/null
> > +++ b/gnu/packages/patches/x11vnc-xfc-null-ptr.patch
> > @@ -0,0 +1,29 @@
> > +From 95a10ab64c2dbbec2c8dad91a5ffb73a0d68474b Mon Sep 17 00:00:00 2001
> > +From: Jonathan Liu <[email-masked]>
> > +Date: Mon, 16 Mar 2020 20:04:06 +1100
> > +Subject: [PATCH] src/cursor: fix xfc NULL pointer dereference
> > +
> > +xfc->width and xfc->height for the XFixes cursor image returned from
> > +XFixesGetCursorImage(dpy) are accessed without first checking that xfc
> > +is not NULL. This can result in the server sometimes crashing when
> > +moving a Google Chrome window.
> > +
> > +Fixes: 37c946191a0f ("Broken cursor bugfix for 64 bit systems (#49)")
> > +Signed-off-by: Jonathan Liu <[email-masked]>
> > +---
> > + src/cursor.c | 2 +-
> > + 1 file changed, 1 insertion(+), 1 deletion(-)
> > +
> > +diff --git a/src/cursor.c b/src/cursor.c
> > +index 39e73a69..74a08c69 100644
> > +--- a/src/cursor.c
> > ++++ b/src/cursor.c
> > +@@ -1311,7 +1311,7 @@ static int get_exact_cursor(int init) {
> > +
> > +         /* retrieve the cursor info + pixels from server: */
> > +         xfc = XFixesGetCursorImage(dpy);
> > +-        {
> > ++        if (xfc) {
> > +             /* 2017-07-09, Stephan Fuhrmann: This fixes an
> > implementation flaw for 64 bit systems.
> > +              * The XFixesCursorImage structure says xfc->pixels is
> > (unsigned long*) in the structure, but
> > +              * the protocol spec says it's 32 bit per pixel
> > diff --git a/gnu/packages/x11vnc.scm b/gnu/packages/x11vnc.scm
> > new file mode 100644
> > index 0000000000..55c51305b3
>
> Since we are using a recent commit, shouldn't these (upstream?) patches
> be unnecessary?  Perhaps we can use an even newer commit that includes
> them?
>
> > --- /dev/null
> > +++ b/gnu/packages/x11vnc.scm
> > @@ -0,0 +1,79 @@
> > +;;; GNU Guix --- Functional package management for GNU
> > +;;; Copyright © 2022 Mehmet Tekman <[email-masked]>
> > +;;;
> > +;;; This file is part of GNU Guix.
> > +;;;
> > +;;; GNU Guix is free software; you can redistribute it and/or modify it
> > +;;; under the terms of the GNU General Public License as published by
> > +;;; the Free Software Foundation; either version 3 of the License, or (at
> > +;;; your option) any later version.
> > +;;;
> > +;;; GNU Guix is distributed in the hope that it will be useful, but
> > +;;; WITHOUT ANY WARRANTY; without even the implied warranty of
> > +;;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +;;; GNU General Public License for more details.
> > +;;;
> > +;;; You should have received a copy of the GNU General Public License
> > +;;; along with GNU Guix.  If not, see <http://www.gnu.org/licenses/>.
> > +
> > +
> > +(define-module (gnu packages x11vnc)
>
> There already is a (gnu packages vnc) module; that'd be a better home.
> We can group all VNC-related packages there.
>
> > +  #:use-module (guix packages)
> > +  #:use-module (gnu packages)
> > +  #:use-module (gnu packages perl)
> > +  #:use-module (gnu packages tcl)
> > +  #:use-module (gnu packages pkg-config)
> > +  #:use-module (gnu packages xorg)
> > +  #:use-module (gnu packages tls)
> > +  #:use-module (guix git-download)
> > +  #:use-module (guix build-system gnu)
> > +  #:use-module ((guix licenses) #:prefix license:)
> > +  #:use-module (gnu packages autotools)
> > +  #:use-module (gnu packages avahi)
> > +  #:use-module (gnu packages vnc)
> > +  #:use-module (gnu packages compression)
> > +  #:use-module (gnu packages image))
>
> Imports should be lexicographically sorted.
>
> > +(define-public x11vnc
>
> When using an unreleased version, it's good to mention why (issues it
> fixes, a very old release, etc.).
> > +  (let ((commit "4ca006fed80410bd9b061a1519bd5d9366bb0bc8")
> > +        (version "0.9.16")
> > +        (revision "1"))
> > +    (package
> > +      (name "x11vnc")
> > +      (version (git-version version revision commit))
> > +      (source (origin
> > +                (method git-fetch)
> > +                (uri (git-reference
> > +                      (url "https://github.com/LibVNC/x11vnc")
> > +                      (commit commit)))
> > +                (file-name (git-file-name name version))
> > +                (sha256
> > +                 (base32
> > +                  "1g652mmi79pfq4p5p7spaswa164rpzjhc5rn2phy5pm71lm0vib1"))
> > +                (patches
> > +                 (search-patches "x11vnc-xfc-null-ptr.patch"
> > +                                 "x11vnc-gcc10-fix.patch"
> > +
> > "x11vnc-scan-limit-access-to-shared-memory.patch"))))
>
> Odd blank line here.  You can try './pre-inst-env guix style x11vnc' to
> see how it would do it, else I'd do it like to avoid breaking the 80
> chars max width:
>
> (search-patches
>  "patch1"
>  "patch2"
>  ...)
>
> > +      (build-system gnu-build-system)
> > +      (arguments
> > +       '(#:phases
> > +         (modify-phases
> > +             %standard-phases
> > +           (add-before 'bootstrap 'delete-premature-configure
> > +             (lambda _ (substitute* "./autogen.sh"
> > +                    ((".*/configure") "")))))))
> > +      (native-inputs (list
> > +                      ;; [optional requirements]
> > +                      ;; perl tk
> > +                      autoconf automake autobuild pkg-config avahi libvnc
> > +                      libx11 libxcomposite libxdamage libxext libxfixes libxi
> > +                      libxinerama libxrandr libxtst
> > +                      openssl xdpyinfo zlib libjpeg-turbo xf86-video-dummy))
>
> The libraries that are used at run time should appear as inputs, not
> native-inputs.  Native-inputs are for build tools or tests.
>
> > +      (synopsis "VNC server for real X displays")
> > +      (home-page "https://github.com/LibVNC/x11vnc")
> > +      (description
> > +       "x11vnc allows one to view remotely and interact with real X
> > +displays (i.e. a display corresponding to a physical monitor, keyboard, and
> > +mouse) with any VNC viewer.  In this way it plays the role for Unix/X11 that
> > +WinVNC plays for Windows.")
>
> I'd remove the useless comparison with windows, perhaps stress that this
> provides a VNC *server* in the description as well.
>
> > +      (license license:gpl2))))
>
> --
> Thanks,
> Maxim
From 379c152a559f7538cb5ec868dc58a03daf592f05 Mon Sep 17 00:00:00 2001
From: Mehmet Tekman <[email-masked]>
Date: Mon, 10 Oct 2022 17:54:21 +0200
Subject: [PATCH] gnu: Add x11vnc.

* gnu/packages/vnc.scm (x11vnc): New variable.
---
 gnu/packages/vnc.scm | 55 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/gnu/packages/vnc.scm b/gnu/packages/vnc.scm
index 84c84aec76..017ac6b343 100644
--- a/gnu/packages/vnc.scm
+++ b/gnu/packages/vnc.scm
@@ -6,6 +6,7 @@
 ;;; Copyright © 2020 Vincent Legoll <[email-masked]>
 ;;; Copyright © 2021, 2022 Tobias Geerinckx-Rice <[email-masked]>
 ;;; Copyright © 2022 Maxim Cournoyer <[email-masked]>
+;;; Copyright © 2022 Mehmet Tekman <[email-masked]>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -24,10 +25,12 @@
 
 (define-module (gnu packages vnc)
   #:use-module (guix build-system cmake)
+  #:use-module (guix build-system gnu)
   #:use-module (guix download)
   #:use-module (guix git-download)
   #:use-module ((guix licenses) #:prefix license:)
   #:use-module (guix gexp)
+  #:use-module (guix git-download)
   #:use-module (guix packages)
   #:use-module (guix utils)
   #:use-module (gnu packages)
@@ -511,6 +514,58 @@ (define openjdk #$(this-package-input "openjdk"))
 ratpoison.")
     (license license:gpl2+)))
 
+(define-public x11vnc
+  ;; The release version of 0.9.16 requires patches to work, so we pin to the
+  ;; latest working commit
+  (let ((commit "3e4dc8ef2985a6e670e1d9649fe55395c2b31039")
+        (version "0.9.16")
+        (revision "1"))
+    (package
+      (name "x11vnc")
+      (version (git-version version revision commit))
+      (source (origin
+                (method git-fetch)
+                (uri (git-reference
+                      (url "https://github.com/LibVNC/x11vnc")
+                      (commit commit)))
+                (file-name (git-file-name name version))
+                (sha256
+                 (base32
+                  "0a120gv9h3whiznlddl0j3nz3400jjgl97znaincm5i2m5pnjifs"))))
+      (build-system gnu-build-system)
+      (arguments
+       '(#:phases (modify-phases %standard-phases
+                    (add-before 'bootstrap 'delete-premature-configure
+                      (lambda _
+                        (substitute* "./autogen.sh"
+                          ((".*/configure")
+                           "")))))))
+      (native-inputs (list autoconf automake autobuild pkg-config))
+      (inputs (list avahi
+                    libvnc
+                    libx11
+                    libxcomposite
+                    libxdamage
+                    libxext
+                    libxfixes
+                    libxi
+                    libxinerama
+                    libxrandr
+                    libxtst
+                    openssl
+                    xdpyinfo
+                    zlib
+                    libjpeg-turbo
+                    xf86-video-dummy))
+      (synopsis "VNC server for real X displays")
+      (home-page "https://github.com/LibVNC/x11vnc")
+      (description
+       "x11vnc allows one to view remotely and interact with real X
+displays (i.e. a display corresponding to a physical monitor, keyboard, and
+mouse) with any VNC viewer.")
+      (license license:gpl2))))
+
+
 (define-public libvnc
   (package
     (name "libvnc")
-- 
2.38.0