Add libslipr dependency to CMakeLists and pipelines

[Phibonacci]

This fixes both pipelines by adding libslirp both to the CMaleLists.txt dependencies and the github pipelines.

Some things to take into account:

• I updated the Ubuntu version to 20.04 because libslirp is not available in 18.04. We could still build it, install it and create a .pc file but I don't see the point
• I applied @nadiaholmquist fix of the Windows pipeline
• I changed the pipeline events to be triggered for every push on every branch and every pull request so we can know if a build is working before a merge on master as most of us will only test it on only one architecture
• I added the libslirp dependency to the CMakeLists.txt of the frontend/ directory which probably does not make so much sense, however that's where everything seemed to be linked. I can move it somewhere else if you'd like to.

[nadiaholmquist]

Looks good, though the CMake scripts in the slurp branch are outdated, could you redo your changes on those on top of the ones from master?

[Phibonacci]

Would you like me to rebase this branch on master or simply copy/paste the CMakeLists.txt from master here and update them?

[rzumer]

We changed the CI workflows to run only on master and PRs because failed runs on WIP branches were annoying people, but I'm fine with reverting that if it doesn't bother others.

Would you like me to rebase this branch on master or simply copy/paste the CMakeLists.txt from master here and update them?

Rebasing would be best.

[Phibonacci]

I rebased my branch on master but github see it conflicts with Arisotura:slirp now. We can directly merge it on master though.
What I changed during the rebase:

• I removed gtk3 dependency and directly added libiconv instead
• I removed the custom cmake build and used the one from Ubuntu 20 instead in a separate commit

[Phibonacci]

Nevermind, something is not working. I cannot run the binary. I'm checking.

[nadiaholmquist]

If I remember right, you only actually need to link libiconv when making a static build.

[Phibonacci]

On linux CMake should return an empty variable since it is part of the libc6. I do not know about the dynamic linked Windows build though, I'll try.

I cannot figure what went wrong and why I cannot start the binary. I'll restart the rebase from scratch and check step by step.

[rzumer]

Sorry, actually rebasing here is a bad idea, my bad for suggesting it, I forgot about the implications of rebasing onto master. If we do it now via PR I think it will mess up the history and make merging the branch back harder.

For the same reason I'm not sure if cherry-picking CMake script changes from master to merge back in slirp is a good idea. You can try doing that and then rebasing slirp on master locally to see if it generates conflicts. If not I don't mind either way. Otherwise I favor the initial approach of ignoring the upstream changes, since conflicts with small commits are simpler to solve.

Sorry again for the misleading comment, you can use git reflog to find the pre-rebase state to revert to.

[Phibonacci]

I see what's wrong. You were right, it's about iconv.

I cannot figure why using ${Iconv_LIBRARIES} and iconv does not produce the same result. Both compile but when compiling with ${Iconv_LIBRARIES} the program does not start, windows shows a window:

The application was unable to start correctly (address).

Even so the variable contains a valib path : /mingw64/lib/libiconv.dll.a

I'll keep iconv then.

[Phibonacci]

@rzumer Alright then. No problem, I'll revert it and add the small changed I mentioned here #681 (comment)

[nadiaholmquist]

Foe static linking you're supposed to use <PREFIX>_STATIC_LIBRARIES, that might be the issue.

[Phibonacci]

There is no ${Iconv_STATIC_LIBRARIES} variable with FindIconv though: https://cmake.org/cmake/help/v3.14/module/FindIconv.html

I tried to print it too. ${Iconv_LIBRARIES} contains a path to a static library anyway.

[nadiaholmquist]

There should be when you use pkg-config?

[Phibonacci]

If we use pkg-config then it needs to be specific to the Windows build because the linux one is not going to find libiconv as it is not a package but part of the libc6. That's why cmake has a specific module for this that's called when doing find_package(Iconv REQUIRED).

For linux it will likely return an empty variable, allowing us to write the same instructions for both linux and windows. Well... in theory, since for some yet unknown reason it fails to run with the variable set by the module.

[nadiaholmquist]

I've already fixed this in a way that works on both Windows and Linux but never got around to doing the PR. I'll find it when I come home tomorrow and post.

[Phibonacci]

Oh, it works on both Linux and Windows since I only link iconv in the Windows build. The current issue you are seeing is simply me removing the gtk3++ dependency and forgetting to add glib2 headers. I'm fixing that.

I was just annoyed I could not find a more elegant way to format the CMakeLists.txt using only one build agnostic declaration. cmake is even able to throw an error when it does not find iconv now, it did not do that before.

[nadiaholmquist]

glib shouldn't be needed, if it is for the slirp branch it's because it was branched before the dependency was removed by my PR here.

[Phibonacci]

Indeed: https://github.com/Arisotura/melonDS/blob/slirp/src/frontend/qt_sdl/Platform.cpp#L40

You will have to remove it later or else this branch won't build. :(

[nadiaholmquist]

I'm sadly not the most knowledgeable about git, but if we merged my branch into the slirp branch and you rebased on that, would it cause issues later?

[rzumer]

I'm sadly not the most knowledgeable about git, but if we merged my branch into the slirp branch and you rebased on that, would it cause issues later?

If your branch is merged in master it might, otherwise it should be fine.

[Phibonacci]

When I rebased the branch earlier I only found merge conflicts in the CMakeLists.txt files so I believe we will be able to merge the branch safely into master.

(right now I'm installing WSL Ubuntu 20 on Windows to figure where glib.h is packaged :'D )

[nadiaholmquist]

Maybe we should rebase the slirp branch on master and work from that, seems like that will avoid the headache of merging all this later?

[rzumer]

That's fine too if you don't mind force pushing.

[Phibonacci]

I believe both rebasing slirp branch and merging master into slirp should represent the same amount of work. It's really about how you want the history to show on master.

[rzumer]

It seems like we use merges over rebases anyway, so I guess the order does not matter much.

[Phibonacci]

Right now this part of the CMakeLists.txt is necessary for the linux pipeline to build:

	find_package(PkgConfig REQUIRED)
	pkg_check_modules(GTK3 REQUIRED gtk+-3.0)

	target_include_directories(melonDS PRIVATE ${GTK3_INCLUDE_DIRS})
	target_link_libraries(melonDS ${GTK3_LIBRARIES})

	ADD_DEFINITIONS(${GTK3_CFLAGS_OTHER})

	add_custom_command(OUTPUT melon_grc.c
		COMMAND glib-compile-resources --sourcedir=${CMAKE_SOURCE_DIR}
				--target=${CMAKE_CURRENT_BINARY_DIR}/melon_grc.c
				--generate-source "${CMAKE_SOURCE_DIR}/melon_grc.xml"
		COMMAND glib-compile-resources --sourcedir=${CMAKE_SOURCE_DIR}
				--target=${CMAKE_CURRENT_BINARY_DIR}/melon_grc.h
				--generate-header "${CMAKE_SOURCE_DIR}/melon_grc.xml")

	if (CMAKE_SYSTEM_NAME STREQUAL "Linux")
		target_link_libraries(melonDS dl slirp Qt5::Core Qt5::Gui Qt5::Widgets)
	endif ()

It worked before because I rebased the branch on master which did not use glib anymore. I could maybe find another more elegant way to write this but why bother if you plan on removing it when merging.

[Phibonacci]

Ok! Last commit. :D

I have spent enough time on this today to understand what are the changes between the building of slirp and master. We cannot use the latest CMakeLists.txt version on slirp since it is too far behind master and requires the glib dependency.

I fixed the pipeline so slirp branch can build again but the rest of the changes will have to be made when merging slirp with master.

[Phibonacci]

Done in #682