lmms icon indicating copy to clipboard operation
lmms copied to clipboard

Update MinGW CI to Ubuntu 20.04

Open messmerd opened this issue 1 year ago • 13 comments

This PR updates the MinGW CI builds to use Ubuntu 20.04 (focal) instead of Ubuntu 18.04 (bionic).

Changes made:

  • Created a new MinGW Docker image: https://github.com/LMMS/lmms-ci-docker/pull/16
  • Fixed the usage of a deprecated Qt method which was causing a build error
  • Fixed a couple issues in build.yml

Benefits:

  • Updates MinGW GCC from 7.4 to 9.3, which will enable support for std::filesystem in all LMMS CI build runners
  • Updates CMake to 3.29.3, which will probably allow us to bump the minimum supported CMake version

Fixes #7256

messmerd avatar May 16 '24 06:05 messmerd

I'm seeing some CMake < 3.5 deprecation warnings, which make me unsure whether the MinGW image received the CMake update.

I may need to push the new base:20.04 and linux:20.04 images to ghcr.io.

messmerd avatar May 16 '24 06:05 messmerd

There's no SDL backend available because the SDL2 target for MinGW is a bit weird and breaks the find module. I was working on this too and have a fix; I'll put it here once I've cleaned it up a bit.

DomClark avatar May 16 '24 07:05 DomClark

This fixes SDL2:

diff --git a/cmake/modules/FindSDL2.cmake b/cmake/modules/FindSDL2.cmake
index 3bad1002e..6e07f7aff 100644
--- a/cmake/modules/FindSDL2.cmake
+++ b/cmake/modules/FindSDL2.cmake
@@ -33,6 +33,18 @@
 find_package(SDL2 CONFIG QUIET)
 
 if(TARGET SDL2::SDL2)
+	# SDL2::SDL2 under MinGW is an interface target for reasons, so we can't get
+	# the library location from it. Print minimal information and return early.
+	get_target_property(sdl2_target_type SDL2::SDL2 TYPE)
+	if(sdl2_target_type STREQUAL "INTERFACE_LIBRARY")
+		unset(sdl2_target_type)
+		if(NOT SDL2_FIND_QUIETLY)
+			message(STATUS "Found SDL2 (found version \"${SDL2_VERSION}\")")
+		endif()
+		return()
+	endif()
+	unset(sdl2_target_type)
+
 	# Extract details for find_package_handle_standard_args
 	get_target_property(SDL2_LIBRARY SDL2::SDL2 LOCATION)
 	get_target_property(SDL2_INCLUDE_DIR SDL2::SDL2 INTERFACE_INCLUDE_DIRECTORIES)

There's another issue in that no dependencies are bundled. This is because binutils isn't installed any more, so CMake can't find objdump. (There's a copy of it in /usr/x86_64-w64-mingw32/bin, which we could use, but I couldn't find a clean way to have CMake find it without hardcoding the directory. The old way is fine if we get that package back.)

Once binutils is installed, a few more libraries need adding to the Windows system library exclude list:

diff --git a/cmake/install/excludelist-win b/cmake/install/excludelist-win
index 17793a113..ac3c47901 100644
--- a/cmake/install/excludelist-win
+++ b/cmake/install/excludelist-win
@@ -3,17 +3,21 @@
 ADVAPI32.dll
 COMCTL32.dll
 comdlg32.dll
+d3d11.dll
 dwmapi.dll
+dxgi.dll
 GDI32.dll
 IMM32.dll
 KERNEL32.dll
 MPR.DLL
 msvcrt.dll
+netapi32.dll
 ole32.dll
 OLEAUT32.dll
 OPENGL32.DLL
 SHELL32.dll
 USER32.dll
+userenv.dll
 UxTheme.dll
 VERSION.dll
 WINMM.DLL

DomClark avatar May 16 '24 22:05 DomClark

There's another issue in that no dependencies are bundled. This is because binutils isn't installed any more, so CMake can't find objdump. (There's a copy of it in /usr/x86_64-w64-mingw32/bin, which we could use, but I couldn't find a clean way to have CMake find it without hardcoding the directory. The old way is fine if we get that package back.)

Would ${CMAKE_FIND_ROOT_PATH}/bin work? (or set(CMAKE_FIND_ROOT_PATH "${CMAKE_FIND_ROOT_PATH}:${CMAKE_FIND_ROOT_PATH}/bin") ?

tresf avatar May 17 '24 15:05 tresf

Unfortunately not - objdump is run from the install script, so CMAKE_FIND_ROOT_PATH isn't set there.

I also tried deducing it from the search paths we already pass for dependencies, but that breaks in a different way. The first thing we try to install is RemoteVstPlugin32.exe, so we look for DLLs in /usr/i686-w64-mingw32/bin. The objdump in there only works for 32-bit EXEs, but once CMake has found a program, it uses the cached location in the future. As a result, when we move on to install lmms.exe next, objdump fails for 64-bit builds.

DomClark avatar May 17 '24 22:05 DomClark

Unfortunately not - objdump is run from the install script, so CMAKE_FIND_ROOT_PATH isn't set there.

I also tried deducing it from the search paths we already pass for dependencies, but that breaks in a different way. The first thing we try to install is RemoteVstPlugin32.exe, so we look for DLLs in /usr/i686-w64-mingw32/bin. The objdump in there only works for 32-bit EXEs, but once CMake has found a program, it uses the cached location in the future. As a result, when we move on to install lmms.exe next, objdump fails for 64-bit builds.

Can't we stash it with CODE(...)?

cmake_minimum_required(VERSION 2.6)
project(Tutorial)

set(MYVAR1 "foobar")

install(CODE "SET(MYVAR1 ${MYVAR1})")

set(MYVAR1 "barfoo")

install(CODE "MESSAGE(STATUS \${MYVAR1})")

tresf avatar May 18 '24 03:05 tresf

.. another possible option is to use a configured CMake input script, then source it back. This was my initial strategy for CPack but decided against it once I learned that values prefixed with CPACK_ were propagated.

tresf avatar May 18 '24 03:05 tresf

The old way is fine if we get that package back.

Oh sorry I'm trying to create a workaround for a missing dependency. Well, it's a fun problem to solve :D.

tresf avatar May 18 '24 03:05 tresf

I can update the Linux image again to add the binutils dependency. EDIT: Nevermind, I didn't read all of the comments above.

messmerd avatar May 18 '24 06:05 messmerd

I can update the Linux image again to add the binutils dependency. EDIT: Nevermind, I didn't read all of the comments above.

Quoting @messmerd /Discord 🗨️ :

@tresf Do you know what I should do about the binutils issue with MinGW? Anything I should change in the Docker image? The Docker image should have binutils-mingw-w64 installed.

Quoting @DomClark

There's another issue in that no dependencies are bundled. This is because binutils isn't installed any more, so CMake can't find objdump. (There's a copy of it in /usr/x86_64-w64-mingw32/bin, which we could use, but I couldn't find a clean way to have CMake find it without hardcoding the directory. The old way is fine if we get that package back.)

I think it's the OS-one, not the mingw one @DomClark's referring to. https://packages.ubuntu.com/focal/binutils

I could also try to leverage the mingw version through some hacks above, but I don't want to add undue complexity, so I'll await direction on that.

tresf avatar May 18 '24 15:05 tresf

The GetPrerequisites module we use for finding dependencies has been deprecated in CMake 3.16 in favour of file(GET_RUNTIME_DEPENDENCIES). While this still uses objdump (or equivalents on other platforms) behind the scenes, it has a somewhat different approach to searching for the tool. As such, I'd rather not spend too much effort fixing up our InstallTargetDependencies/InstallDependencies modules as they currently stand if we might redo them anyway. Then again, it means a hacky fix would matter less.

I like installing binutils as a solution, as it addresses the fundamental difference between the two environments, putting us back where we were before. The downside is the redundancy of having yet another version of objdump (and other things in that package), but it's more of an inelegance rather than an actual problem. I expect using install(CODE) to set CMAKE_FIND_ROOT_PATH in the install script would work too, although I haven't tried it. Overall, I would rather the requirements of the code determined what we install for CI, rather than vice versa - it seems odd to write workarounds for the CI environment given that we have control over it - but I don't think it matters too much.

DomClark avatar May 18 '24 23:05 DomClark

@DomClark what about https://github.com/LMMS/lmms/blob/master/cmake/modules/DefineInstallVar.cmake

# This functions forwards a variable to
# the install stage.

tresf avatar May 19 '24 23:05 tresf

That's equivalent to install(CODE), which is what it uses internally. It was written in order to use generator expressions with install(CODE) while still supporting CMake < 3.14, falling back to configuring a file and reading it from the install script. It's redundant now we have a much newer CMake version available.

DomClark avatar May 20 '24 07:05 DomClark