Some Debian patches to TigerVNC that might also be useful upstream

[JoachimFalk]

On 13.12.24 22:45, Joachim Falk wrote:

Hi Pierre,

the Debian TigerVNC package carries some patches that might also be
useful to you.

0010-fix-xtigervnc-build.patch is need for building Xvnc out of source
0102-fix-option-dashes-in-manpages-to-shutup-lintian.patch
Some option '-' in man pages are rendered as Unicode dashes not ASCII
'-', fix this.
0151-make-cmake-enable-options-mandatory-if-turned-on.patch
If a feature is explicitly requested via the cmake command line,
error out if necessary libraries are missing and do not silently
disable the requested feature.
0225-add-missing-fields-to-vncviewer-metainfo.patch
Some field where missing in the appinfo metadata. I added you
as primary developer and also added the TigerVNC homepage.
I hope, that is OK with you.
0220-remove-systemd-service-obsolete-syslog-target.patch
Removed obsolete syslog.target from [email protected].
The syslog.target is obsolete in Debian. I am relatively sure
that this is also the case for other distributions.

On 16.12.24 08:07, Pierre Ossman wrote:

Thanks. Getting everyone synced up on the same code base is always nice.

Do you think you could submit them as pull request(s) via GitHub
instead? It's easier to discuss any details there.

Done as requested.

Best,

Joachim

[CendioOssman]

Thanks for working on getting this in sync. There are some questions I'd like to discuss before merging, though.

[CendioOssman]

I'm unsure of the use case here. I would assume that when Xorg is built out of tree, then so is TigerVNC. And it would be unclear what that build directory is.

What is the scenario where you find this change helpful?

[JoachimFalk]

Adjusted the commit message to explain:

"Added reasonable defaults for TIGERVNC_BUILDDIR in the build of Xvnc, assuming that when Xvnc is building out of source, then so are the TigerVNC libraries and that the same relative directory relations will be preserved in the out-of-source build tree as are present in the source tree."

[CendioOssman]

Thanks. I'm afraid I'm still not quite understanding the scenario. Are you saying the setup is:

TigerVNC source: ~/devel/tigervnc
TigerVNC build dir: ~/devel/tigervnc/build
Xvnc source: ~/devel/tigervnc/unix/xserver
Xvnc build dir: ~/devel/tigervnc/build/unix/xserver/

?

A reasonable setup, but not an obvious one. I don't see anyone else using this structure.

Still, I don't think it's a worse default than what we currently have, so I'm okay with merging it.

The commit message doesn't follow normal git conventions, though. Could you make sure it has a maximum 50 character title, follow with a body? Same thing for the other commits in this PR.

[JoachimFalk]

Yes, exactly like this. I changed the commit messages again to use standard format and used your example to make the meaning cleaner.

[CendioOssman]

I sympathize with the goal here, but I'm concerned that the method gets a bit convoluted. Leaving booleans without a clear value, and doing string comparisons with them, can be a bit confusing and easy to get wrong.

Are you aware of any alternative models to solve this issue?

[JoachimFalk]

I rewrote the code to use NOT DEFINED instead.

[CendioOssman]

I don't think this gives a stable behaviour. I guess the option is now only presented if you've also specified it on the command line?

Perhaps we can do something with the default value instead?

[CendioOssman]

How about this:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 5b91d84ed..a5dc8566f 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -117,6 +117,13 @@ if(APPLE)
   add_definitions(-D__ASSERT_MACROS_DEFINE_VERSIONS_WITHOUT_UNDERSCORES=0)
 endif()
 
+#### Check for required and optional libraries ####
+
+macro(trioption VAR DESC)
+  set(${VAR} AUTO CACHE STRING "${DESC}")
+  set_property(CACHE ${VAR} PROPERTY STRINGS AUTO ON OFF)
+endmacro()
+
 # X11 stuff. It's in a if() so that we can say REQUIRED
 if(UNIX AND NOT APPLE)
   find_package(X11 REQUIRED)
@@ -141,13 +148,17 @@ find_package(ZLIB REQUIRED)
 find_package(Pixman REQUIRED)
 
 # Check for gettext
-option(ENABLE_NLS "Enable translation of program messages" ON)
+trioption(ENABLE_NLS "Enable translation of program messages")
 if(ENABLE_NLS)
   # Tools
   find_package(Gettext)
 
   # Gettext needs iconv
-  find_package(Iconv)
+  if(ENABLE_NLS STREQUAL "AUTO")
+    find_package(Iconv)
+  else()
+    find_package(Iconv REQUIRED)
+  endif()
 
   if(ICONV_FOUND)
     # Headers and libraries (copied from licq)
@@ -173,6 +184,10 @@ if(ENABLE_NLS)
       set(CMAKE_REQUIRED_LIBRARIES)
       set(CMAKE_REQUIRED_FLAGS)
     endif()
+
+    if(NOT GETTEXT_FOUND AND NOT ENABLE_NLS STREQUAL "AUTO")
+      message(FATAL_ERROR "Could not find GETTEXT")
+    endif()
   endif()
 
   if(NOT GETTEXT_FOUND OR NOT ICONV_FOUND)
@@ -181,7 +196,7 @@ if(ENABLE_NLS)
   endif()
 endif()
 
-option(ENABLE_H264 "Enable H.264 RFB encoding" ON)
+trioption(ENABLE_H264 "Enable H.264 RFB encoding")
 if(ENABLE_H264)
   if(WIN32)
     add_definitions("-DHAVE_H264")
@@ -195,7 +210,11 @@ if(ENABLE_H264)
       add_definitions("-DHAVE_VIDEO_PROCESSOR_MFT")
     endif()
   else()
-    find_package(Ffmpeg)
+    if(ENABLE_H264 STREQUAL "AUTO")
+      find_package(Ffmpeg)
+    else()
+      find_package(Ffmpeg REQUIRED)
+    endif()
     if (AVCODEC_FOUND AND AVUTIL_FOUND AND SWSCALE_FOUND)
       set(H264_INCLUDE_DIRS ${AVCODEC_INCLUDE_DIRS} ${AVUTIL_INCLUDE_DIRS} ${SWSCALE_INCLUDE_DIRS})
       set(H264_LIBRARIES ${AVCODEC_LIBRARIES} ${AVUTIL_LIBRARIES} ${SWSCALE_LIBRARIES})
@@ -283,17 +302,25 @@ if(BUILD_VIEWER)
 endif()
 
 # Check for GNUTLS library
-option(ENABLE_GNUTLS "Enable protocol encryption and advanced authentication" ON)
+trioption(ENABLE_GNUTLS "Enable protocol encryption and advanced authentication")
 if(ENABLE_GNUTLS)
-  find_package(GnuTLS)
+  if(ENABLE_GNUTLS STREQUAL "AUTO")
+    find_package(GnuTLS)
+  else()
+    find_package(GnuTLS REQUIRED)
+  endif()
   if (GNUTLS_FOUND)
     add_definitions("-DHAVE_GNUTLS")
   endif()
 endif()
 
-option(ENABLE_NETTLE "Enable RSA-AES security types" ON)
+trioption(ENABLE_NETTLE "Enable RSA-AES security types")
 if (ENABLE_NETTLE)
-  find_package(Nettle)
+  if(ENABLE_NETTLE STREQUAL "AUTO")
+    find_package(Nettle)
+  else()
+    find_package(Nettle REQUIRED)
+  endif()
   if (NETTLE_FOUND)
     add_definitions("-DHAVE_NETTLE")
   endif()

[JoachimFalk]

OK, this is much cleaner. I adopted this.

[CendioOssman]

Looks good! Thanks again!