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

Modern CMake overhaul for GZDoom + ZMusic submodule #1401

Closed
wants to merge 1 commit into from

Conversation

Blzut3
Copy link
Member

@Blzut3 Blzut3 commented May 31, 2021

I've been picking away at this for awhile so I wouldn't be too surprised if I forgot something. As with ZMusic, sorry about the completely unreadable diff, but such is the nature of this effort.

This was a lot harder modernize than ZMusic since there was a ton of legacy stuff and not really feasible to just throw it out and start fresh, but I think I got something I'm mostly happy with. Of course the big changes are CMakeLists.txt and src/CMakeLists.txt.

For the root CMakeLists.txt, the goal was to hoist as much as possible into modules and move target setup elsewhere. To that end we now have a CMakeLists.txt specifically for vendored libraries, a few new files in cmake/, and the rest got filtered down into src/.

In src/ I tried to minimize the amount of branching, or perhaps more accurately branching and then doing the same branch hundreds of lines later in the same file. Modern CMake lets us also avoid having to add to variables since we can just append stuff to targets. I've created sections marked with BEGIN and END (my text editor can fold on these, although not sure if there's a more compatible syntax to get folding) and that folding has been useful to me so I would say the sorting has been a success.

I'm not too happy about the source list variables still hanging around they way they are, but I really couldn't think of a better way to handle the situation. But what I was able to do is mostly move the logic bits below the entire source listing. (Only thing I kept in line was the platform selection since I couldn't figure out a nice clean way to defer that to later.) There were previously some conditionally compiled sources that weren't always added to the Visual Studio project, and that's fixed now.

Of course ZMusic is now a submodule. One is free to use a system copy of ZMusic (or otherwise import a separately compiled ZMusic), and it's not required to have the submodule cloned to do that. I'm not sure if we still want to keep the prebuilt lib files here now that using the submodule is an option, but those are still supported as well. The condition upon which they are used may need some tweaking though depending on what we want to do.

I put some initial CPack support in which can generate deb packages and on Windows produce a zip archive. The deb package is mostly where it needs to be, but there's more to be done here. Just no need to hold off the rest of this for that to be done.

@coelckers
Copy link
Member

Please remove the submodule.
The one for widepix has been nothing but trouble, I won't deal with them anymore.
widepix will also be properly integrated once Nash's work is done.

@Blzut3
Copy link
Member Author

Blzut3 commented May 31, 2021

Does it make no difference that this submodule is optional? My understanding is your concerns are about stuff like bisecting acting weird, but the way I implemented this nothing would stop you from bisecting a clone that doesn't have the submodule.

The reason I ask is one of the problems I was trying to solve is putting a pointer to the desired ZMusic version in this repo for building the dev builds. This can probably be solved another way, but it seems like the submodule being optional should be a decent compromise?

@Blzut3
Copy link
Member Author

Blzut3 commented Jun 1, 2021

Was trying to think about what other projects do to solve this problem and GCC's contrib/download_prerequisites comes to mind. Basically I could write a shell script (compatible with git-bash on Windows) which users can choose to use to automatically pull down ZMusic for in-tree compilation if they so choose. This would have the advantage of when you download the archive of the source code without git the pull down script would still be available. However, there is the reality that such script is just duplicating the functionality of git submodules without any of the git integration. Something you'd probably see as a positive given your position on the feature.

Ultimately up to you what route to go down, or we can discuss this further. There are two problems I'm trying to solve with a semi-automated solution. First is that users find building GZDoom frustrating due to the ZMusic dependency not being vendored (numerous forum posts and an issue ticket here back this claim up). Second is what I mentioned int he last post in that I would like for this repo to suggest what ZMusic version to build for any given version of GZDoom. ZMusic is ABI stable, but there are still reasons such as the auto builds where this information can be useful.

There is a third problem that allowing ZMusic to optionally be built in tree solves and that is it allows CPack to easily build the deb packages. But in-tree building can be supported with manually dropping the ZMusic source tree in the right location.

Again to reiterate, no matter what route is taken, the script is written to allow the libraries/zmusic directory to be empty and function just fine with an imported build as done today.

@alexey-lysiuk
Copy link
Collaborator

I was against usage of submodules initially, and didn’t change my mind since then.

Regarding ZMusic dependency, it’s not our problem that someone cannot build GZDoom/Raze because of it.
If they have no idea what they are doing, just download a binary package.
Willingness to build from source code is nice, but it requires some knowledge. It’s not a blind shell commands copy-paste job.

On the other hand, if ZMusic wasn’t found, it’s OK to grab its sources to build in-tree.
However, this should be done in pure CMake, I think.

@Blzut3
Copy link
Member Author

Blzut3 commented Jun 1, 2021

If it's not our job to make compiling easy, then why do we vendor any of the libraries?

Doing the fetch in CMake is certainly a possibility. I'm not a huge fan of doing non-build stuff in CMake since the language is very not designed for that, but it can be done.

@alexey-lysiuk
Copy link
Collaborator

If it's not our job to make compiling easy, then why do we vendor any of the libraries?

Our job is to make things work. If we can make it easy without too much hassle, I'm fine with it. Otherwise, we may end up with a custom app that offers a single 'Do everything, and make me happy' button.

Doing the fetch in CMake is certainly a possibility. I'm not a huge fan of doing non-build stuff in CMake since the language is very not designed for that, but it can be done.

I meant something like FetchContent module.

@coelckers
Copy link
Member

GZDoom can be built on Windows without any intervention or additional setup. It's only when running the compiled EXE that the .dll's have to be grabbed from a binary package. That's the case with or without ZMusic.

What I found far more ironic is how the process broke down for some Linux users when they had to think a little bit out of the box with ZMusic. The number and the kind of complaints this received was a little bit ridiculous.
I agree with @alexey-lysiuk that it's not the developer's task to spoon-feed the perfect solution. The goal should be to allow configuring the project with reasonable effort. If that is too much to ask, don't self build, plain and simple.

But submodules are not the solution. The problems they cause are often a lot bigger than some minor inconvenience in the build process.

@Blzut3
Copy link
Member Author

Blzut3 commented Jun 2, 2021

There has been an ask for me to provide Linux dev builds for GZDoom and Raze, and the reason that I stopped doing the Mac builds was because I wasn't ready to come up with a solution for the library problem, so dismissing the issue as "it's fine on the one platform I build on" is missing the point. A lot of this is to make my life easier even though I'm perfectly capable of figuring out how to link two builds together. Heck I compiled fluidsynth for you guys.

Anyway @coelckers any comment on either of the two solution proposed (FetchContent or my prereqs script idea) are acceptable to you? I'm not personally a huge fan of FetchContent, but it does technically meet all my stated requirements. So if that's what you guys find acceptable, then that's what I'll do.

@coelckers
Copy link
Member

What's the problem with Mac? I've never done a release build on that platform, but my approach on Windows has always been to use the last released binary, update all files that have changed and release that.

And what, particularly, makes ZMusic such a problem? What's with the other dependencies that aren't part of the build itself, like OpenAL, FluidSynth or libsndfile?

@Blzut3
Copy link
Member Author

Blzut3 commented Jun 2, 2021

The problem is tedium which isn't bad if you're doing one platform, but do a bunch and you inevitably forget things. It's why I vendored SDL2, SDL2_mixer, ogg, flac, opus, and vorbis in ECWolf, at 20-something binaries per release it was getting a bit out of hand. Now I just configure CMake and it does the work for me.

As for why ZMusic and not the others.

  • Firstly, it's a hard prereq for building GZDoom. You have a workaround for Windows, but I'm looking to make it similarly painless for all platforms for my sanity, and as a side effect everyone else.
  • Secondly, we control both repos so I have the ability to make the integration seamless without the need for hacks or maintaining our own fork of the upstream repo. It just works like it should whether you're using ZMusic out of tree or in tree.
  • Thirdly, many people don't want to use my apt repo because they've been told using third party repos is dangerous (I think the rationale is bogus but that's neither here nor there). ZMusic is not a library that distros provide so we need to provide it in the deb package for GZDoom/Raze. (So you can treat the deb like an msi and just double click and install.) I want to get away from hand building deb packages, having the option to do a unified build allows me to build the packages with CPack and not have to make people download multiple debs.
  • Finally you're making the false assumption that I wouldn't like to have a pointer to recommended versions of those libraries as well. There are things that make this more complicated (basically repeating point two here) that this isn't something I'm proposing right now. But yes, it's something that's on my mind if I can find a realistic way to do it. I will note that vendoring fluidsynth is pretty much out of the question since its dependencies would be incredibly difficult to put in a monolithic build. I don't see fluidsynth as particularly important here though since it's just one of many soft synths that ZMusic supports so you're not giving up functionality by not having it. It's more of a plug-in.

In the case of OpenAL, libmpg123, and libsndfile what I would like at a minimum is a machine readable document that just documents "this is the repo and tag of these dependencies that GZDoom/ZMusic recommends being used with." I could then sort out on the build server how to produce the libraries from that information. This would be useful for doing automatic Mac builds, not so much on Linux since we just have the user download those libraries from their distro. I could dig more into this topic, but I'm not sure if it's productive to this thread.

Now a similar file with those two pieces of info would be fine for ZMusic as well for my specific purposes technically speaking. That said, it would be really sad not to have integration because you've yet to state any reasons why the proposed solutions don't work for you. Like OK, I get that my initial solution is out since git submodules left such a sour taste that you can't even be bothered to update the submodule pointers even if your day to day clone doesn't have them checked out. But you have two other proposals that simply require documenting which version you recommend either in some manifest (my shell script) or in CMake (FetchContent). Why are either of these too much work?

@coelckers
Copy link
Member

Submodules are not a solution. I had to find out the hard way that they are far too broken to be used in any project that needs active maintenance. Apparently Rachael came to the same conclusion, judging by her post in the forum. So if you can provide something that works without submodules, fine. But if your solution requires submodules to work, count me out.

@Blzut3
Copy link
Member Author

Blzut3 commented Jun 3, 2021

All right, I think I should have time on Sunday to put something together.

@alexey-lysiuk
Copy link
Collaborator

I encountered a pretty specific issue with this change. As it requires fairly long explanation, and it's tightly coupled with ZMusic, I put it in a separate report ZDoom/ZMusic#28.

- Untangled src/CMakeLists.txt and sorted into sections with defined purpose.
  These sections are marked with BEGIN and END which is supported by folding
  in some text editors (Kate for example).
- Reduced global variable pollution in project, prefering target properties
  where feasible.
- Removed dead code and legacy CMake 2.4-isms.
- Allow ZMusic source to be provided in libraries/zmusic as a submodule. This
  permits distributing GZDoom source independent of ZMusic but still allowing
  GZDoom to be easily compiled with ZMusic in-tree. Also will use system
  installed ZMusic if available as a CMake module.
- Initial support for CPack, although there's still more to be done to ensure
  that the DEB packages are compliant.
@Blzut3
Copy link
Member Author

Blzut3 commented Jun 7, 2021

Updated to use FetchContent. Although I'm not sure I'm 100% satisfied with the way the automatic fetching (disabled by default on MSVC) is setup. Specifically in the case of alexey-lysiuk's ticket there the new way completely masks the error since CMake doesn't differentiate between find_package not finding the config and the config failing to find dependencies. There's likely some better logic I can do here, but I think I need to sleep on it.

Of course would love to hear any other feedback in the mean time.

@alexey-lysiuk
Copy link
Collaborator

With non-MSVC configuration, the new version always fetches source code to build ZMusic even if headers and library are found at prefix path. Is it supposed to work this way? The initial version didn't have this "feature".

@madame-rachelle
Copy link
Collaborator

I am actually quite eager for this, as I would like to build .deb packages for non-x86 platforms (including the Pi) and have found hand-building those things to be quite a pain in the butt, especially after the ZMusic split, which is why I haven't provided Pi packages or Linux packages period since that one person complained that my binary wouldn't work for them with Raze.

This would even allow for the possibility of doing dev build nightlies for, at the very least, Linux, and our Linux user base is growing so I think it would be nice to have something for them when they want to try out new features between releases.

@madame-rachelle
Copy link
Collaborator

As for the submodules thing - yes, they're great when they work properly, but they are too prone to user error to be considered reliable by any degree. Sure the user error was on my end this time - but I have heard many ... too many stories by now ... about other users having issues with it. Even something that pulls ZMusic by tag, externally, would be far better than submodules because they would fulfill the goals submodules intially set out to achieve, but with the added bonus of being more automatic and managed.

Sorry but - Git just kinda fucked submodules up from the very beginning. When the widepix project is done, Graf is quite right that the submodule will be removed.

@madame-rachelle
Copy link
Collaborator

It's a shame that all the work was done in this PR and it's simply gathering dust right now, enough that it is building up into merge conflicts. I don't imagine they'll be too difficult to deal with right now, but as time goes on, they will be more and more difficult to deal with unless this is merged.

Regarding submodules - my stance has changed quite a bit since we started using them. I don't like them at all, and was glad when widepix got merged into the GZDoom repo properly. When I did widepix updates through the submodule even Graf complained that he wasn't getting the updates on his end unless he did something to reset the submodule.

So in the end, the benefits of having a self-contained repository seem to far outweigh the costs of having the submodules. There was an idea to try subtrees but I have no experience with that right now.

Anyway - like I said - I really do hope to see this merged - before it gets to a point of unmaintainability as a separate branch.

@Blzut3
Copy link
Member Author

Blzut3 commented Oct 28, 2021

Merge conflicts shouldn't be hard at all to solve, but yeah weekends have been plugged so haven't gotten back at this.

@coelckers
Copy link
Member

I can't sift though this wall of changes. It may make sense to resubmit in smaller portions that are easier to check. In its current state I won't merge it for fear of breaking stuff.

@coelckers coelckers closed this May 31, 2022
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.

None yet

4 participants