tenacityteam / tenacity-legacy

THIS REPO IS NOT MAINTAINED ANYMORE. Please see https://codeberg.org/tenacityteam/tenacity for Tenacity, which is maintained.
https://tenacityaudio.org
Other
6.8k stars 262 forks source link

Support GTK2, improve different wxWidgets ports handling #574

Open leio opened 2 years ago

leio commented 2 years ago

Guidelines

Version/Commit hash

807b710c742d83a118bda57e6c66995cfdafecd8

Describe the bug.

  1. Configure against a gtk2 using wxGTK wx-config
  2. Compile it
  3. Compilation fails due to mismatches from including gtk/gtk.h from gtk3 in tenacity code, while wxGTK headers have pulled in some gtk2 stuff

Expected behavior

The build system should check for wxWidgets selected configuration first and then do the gtk checks to match wxGTK configuration

OS

Linux

Additional context

The build succeeds and works after a hacky patch is applied to cmake as follows:

$ git diff 
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 08f1ff262..cd30bffb2 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -695,7 +695,7 @@ endif()

 if(NOT CMAKE_SYSTEM_NAME MATCHES "Darwin|Windows")
     find_package(GLIB REQUIRED)
-    find_package(GTK 3.0 REQUIRED)
+    find_package(GTK 2.0 REQUIRED)

 endif()

diff --git a/cmake-modules/FindGTK.cmake b/cmake-modules/FindGTK.cmake
index abb4b863f..2490d5a0e 100644
--- a/cmake-modules/FindGTK.cmake
+++ b/cmake-modules/FindGTK.cmake
@@ -60,6 +60,8 @@ This will define the following variables in your project:
   whether GTK 4 was detected
 ``GTK_3``
   whether GTK 3 was detected
+``GTK_2``
+  whether GTK 2 was detected
 ``GTK_VERSION``
   the version of GTK.
 ``GTK_SUPPORTS_BROADWAY``
@@ -79,16 +81,24 @@ if (NOT DEFINED GTK_FIND_VERSION)
     message(FATAL_ERROR "No GTK version specified")
 endif ()

-if (GTK_FIND_VERSION VERSION_LESS 3.90)
+if (GTK_FIND_VERSION VERSION_LESS 2.90)
+    set(GTK_PC_MODULE "gtk+-2.0")
+    set(GTK_PC_UNIX_PRINT_MODULE "gtk+-unix-print-2.0")
+    set(GTK_4 FALSE)
+    set(GTK_3 FALSE)
+    set(GTK_2 TRUE)
+elseif (GTK_FIND_VERSION VERSION_LESS 3.90)
     set(GTK_PC_MODULE "gtk+-3.0")
     set(GTK_PC_UNIX_PRINT_MODULE "gtk+-unix-print-3.0")
     set(GTK_4 FALSE)
     set(GTK_3 TRUE)
+    set(GTK_2 FALSE)
 else ()
     set(GTK_PC_MODULE "gtk4")
     set(GTK_PC_UNIX_PRINT_MODULE "gtk4-unix-print")
     set(GTK_4 TRUE)
     set(GTK_3 FALSE)
+    set(GTK_2 FALSE)
 endif ()

 find_package(PkgConfig QUIET)

Instead of this, in particular for the toplevel CMakeLists part, we need to detect the wxWidgets configuration, and then make the gtk check as appropriate, something like this in prototype code (I just don't know nor really want to know cmake enough to make it happen), combined with the FindGTK module patch from above:

config = exec(wx-config --selected-config)
if config.starts_with('gtk'):
  find_package(GLIB REQUIRED)
  if config.starts_with('gtk2'):
    find_package(GTK 2.0 REQUIRED)
  elif config.starts_with('gtk3'):
    find_package(GTK 3.0 REQUIRED)
  else:
    abort("Unknown wxGTK configuration")
  endif
endif

Then we could actually also go deeper with this and also check it for other values, replacing the incorrect platform detection from src/CMakeLists.txt that assumes Windows is always wxMSW, Darwin is always wxMac and the rest is always wxGTK - e.g. wxGTK can work on Mac as well, or it would just fail at compilation stage instead of configuration stage when wxX11 is picked via WX_CONFIG right now. So the toplevel if in the prototype code above could be a switch case (if cmake has that instead), which will be happy with gtk, msw and osx (whatever their wx-config prefixes actually are), but error out early in case of anything else. And the wxIS_GTK and other booleans could be set up there, instead of the platform-specific hack in src/CMakeLists.txt.

Another project doing something similar in autotools can be seen here: https://svn.filezilla-project.org/filezilla/FileZilla3/trunk/configure.ac?revision=10360&view=markup#l597

This issue is not a duplicate

Be-ing commented 2 years ago

Why is compatibility with an already very obsolete library helpful? GTK3 is already outdated.

leio commented 2 years ago

I don't really care if we support it or not, but it shouldn't fail deep into the build phase, but error out at configure time. At that point it's basically a matter of either writing an error in that case, or just passing 2.0 instead of 3.0 to the find_package call (with the find module patch above).

So personally I'm more interested in cleaning up the platform vs wxWidgets port mixups in the build system, and fixing compilation failures (either by fixing it for gtk2, or just cleanly erroring out at configure phase).

Be-ing commented 2 years ago

I'd be in favor of a fatal error message at configure time if only GTK2 is found.

Zorgodon commented 2 years ago

I personally much prefer the look of GTK2, and compile the original Audacity and Filezilla (as in OP) against it. I don't see why this fork should demand gtk3 when wxwidgets guarantees compatibility with both toolkits.

Be-ing commented 2 years ago

Well are you volunteering to fix every bug due to GTK2?

caughtquick commented 2 years ago

Supporting GTK2 adds even more of a maintenance load. I'm in favor of erroring out before the build.

leio commented 2 years ago

I have no problem in a best-effort basis of supporting it, until we actually write code that won't work with gtk2 (requires API added in some gtk3 version). But that would mean mostly just reacting to issues filed when they are easy enough to fix up (e.g. accidental compile failure or something easy), and not any kind of deeper gtk2 work. So no making things look or work better in gtk2 specifically, or near-impossible pipedreams such as proper HiDPI support with gtk2. And no fixing wxGTK gtk2-specific bugs. But I also have no problem with dropping the support - gtk2 itself is unmaintained, afterall.

We don't have much gtk specific code, and the platform-specific code we will write (e.g. wxFileDialog that opt-in supports GtkFileChooserNative on gtk3), we need to do basic gtk2 support anyways, in order to successfully upstream those changes.

Be-ing commented 2 years ago

proper HiDPI support with gtk2

Ardour still has some issues with high DPI scaling with GTK2.

Be-ing commented 2 years ago

personally much prefer the look of GTK2

I don't think this is a good reason for adding a huge maintenance burden.

leio commented 2 years ago

proper HiDPI support with gtk2

Ardour still has some issues with high DPI scaling with GTK2.

Yes, I'm saying we'd be ignoring any and all such fundamental issues with GTK2; GTK2 support would just be kept compiling on a best effort basis (not officially supported). It might prove beneficial for myself too for when I'm working on some code that is intended to get upstreamed to wxWidgets, as there it'll probably need to work with GTK2 too to some extent, so it can be perhaps tested easier (just that area) against GTK2. It could be behind a heavy warning, or even error out by default, but have a I_KNOW_GTK2_IS_NOT_SUPPORTED=1 override or something.

nbsp commented 2 years ago

I also think a FORCE_GTK2 override is the best solution. We shouldn't support GTK2, but there's no harm in allowing people to enable it.

Be-ing commented 2 years ago

Okay, we could have a CMake option to allow building with GTK2. I'm not volunteering to implement it.

I_KNOW_GTK2_IS_NOT_SUPPORTED=1 override

:laughing: I like this idea for naming the CMake option.

nbsp commented 2 years ago

:laughing: I like this idea for naming the CMake option.

The commit should be titled Allow users to shoot themselves in the foot

Be-ing commented 2 years ago

What exactly do we mean by "not supported"? I think that should mean "fix bugs yourself if you want but please don't bother us with bug reports that only affect GTK2".

nbsp commented 2 years ago

The way I was thinking about it, it's "we're not going to look at GTK2-exclusive bugs, and bugfixes might take longer to review".