Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update MinGW CI to Ubuntu 20.04 #7259

Merged
merged 8 commits into from
May 23, 2024
Merged

Update MinGW CI to Ubuntu 20.04 #7259

merged 8 commits into from
May 23, 2024

Conversation

messmerd
Copy link
Member

@messmerd messmerd commented May 16, 2024

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

Changes made:

Benefits:

  • Updates MinGW GCC from 7.4 to 9.3, which will enable support for std::filesystem in all our build runners
  • Updates Qt from 5.9.5 to 5.15.6, which I believe will make our Linux CI the lowest Qt version with 5.12.8.
  • Updates SDL from SDL 1.2.x to SDL 2.0.20, which means we only use SDL2 now
  • Updates CMake to 3.29.3, which will probably allow us to bump the minimum supported CMake version
  • All of our Docker images fully migrated to ghcr.io

Fixes #7256

@messmerd
Copy link
Member Author

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.

@DomClark
Copy link
Member

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
Copy link
Member

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

@tresf
Copy link
Member

tresf commented May 17, 2024

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

@DomClark
Copy link
Member

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.

@tresf
Copy link
Member

tresf commented May 18, 2024

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
Copy link
Member

tresf commented May 18, 2024

.. 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
Copy link
Member

tresf commented May 18, 2024

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.

@messmerd
Copy link
Member Author

messmerd commented May 18, 2024

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

@tresf
Copy link
Member

tresf commented May 18, 2024

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.

@DomClark
Copy link
Member

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.

@tresf
Copy link
Member

tresf commented May 19, 2024

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

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

@DomClark
Copy link
Member

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.

@tresf tresf requested a review from DomClark May 23, 2024 06:04
@tresf tresf merged commit 2e65453 into LMMS:master May 23, 2024
9 checks passed
@messmerd messmerd deleted the mingw-ci-update branch May 23, 2024 18:15
B0ney added a commit to B0ney/lmms that referenced this pull request May 26, 2024
sakertooth pushed a commit to sakertooth/lmms that referenced this pull request May 27, 2024
Update MinGW CI to Ubuntu 20.04
* Use ghcr.io/lmms/linux.mingw:20.04
* Fix deprecation in ClipView.cpp
* Fix ccache and simplify git configuration
* Apply patch by @DomClark for MinGW's SDL2 target
* Update excludelist-win
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update MinGW CI to 20.04
3 participants