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

games-roguelike/cataclysm-dda: fixes #111

Merged
merged 2 commits into from
May 28, 2024
Merged

Conversation

nethershaw
Copy link
Contributor

Hello! I come bearing gifts.

There are two separate fixes offered here for the Cataclysm: DDA ebuild.

  • Compiling with USE=lto was broken because of reorganized dependencies in ::gentoo. This fix removes a dependency previously required for LTO that is required no longer, and suggests a more Gentoo-esque means of involving alternate linkers if the user so desires. I benchmarked this ebuild with GCC and Clang and four different linkers (BFD, Gold, LLD, and Mold) to reach my conclusions.
  • Compiling with GCC 13 was broken because of reorganized libraries in the C++ standard namespace.

Full details and justifications are in the commit messages.

If `USE=lto` was enabled, this would pull in `sys-devel/llvm[gold]`,
which has been removed from ::gentoo in favor of
`sys-devel/binutils[gold]` and cause the ebuild to fail with
unresolvable dependencies.

At the same time, upstream has made use of the Gold linker optional by
way of the build flag `GOLD`.

Empirically, when comparing Clang+LTO builds using ld.bfd and ld.gold,
build times during the linking phase are the same within error margins.
This does not justify choosing the option. This commit disables the
upstream Makefile's preference for it, allowing the ebuild to set its
own LDFLAGS if desired.

The Gold linker has generally fallen out of favor because its patrons
have moved on to LLVM, and active development elsewhere has continued in
ld.lld and ld.mold. These linkers are both valid options for this ebuild
with or without LTO enabled, but should be left for the user to enable
by way of Portage environment controls; e.g.,

/etc/portage/env/mold:
```
LDFLAGS="${LDFLAGS} -fuse-ld=mold"
```

/etc/portage/package.env/cataclysm-dda:
```
games-roguelike/cataclysm-dda mold
```

The above will, assuming the user has installed `sys-devel/mold`, ask
the package to be linked using `ld.mold`. This also happens to offer an
enormous speedup when compiling with GCC, and decouples the choice of
linker from the choices for LTO and compiler.
cf. CleverRaven/Cataclysm-DDA#66195

This packages a patch from the upstream PR, which simply includes the
`cstdint` library in a header file. Due to changes in the way C++
Standard Library headers are organized in GCC 13, the build fails
without this include.

Note that current live builds and future release builds already include
the fix. This commit should be reverted when release "0.G" is
superseded and this backporting patch is no longer required.
@leycec
Copy link
Owner

leycec commented May 28, 2024

Phenomenal! You have born gifts, indeed. Thanks so much for this tremendous changeset. It's in! It's so in, @nethershaw.

Incidentally, do you have more free time than is comfortable? If so, I (and probably many other Gentooers) would be eternally grateful if you would consider resolving long-standing Gentoo bug 699288 by submitting a Gentoo Portage PR officially adding C:DDA to Portage. Now, I know what you're thinking:

"Hell no, dawg."

But just hear me out. I'm quietly contemplating the closure of this Gentoo overlay at the end of this year. Why? @beartype, mostly. I 😻 Gentoo, but I 😻 Python even more. @beartype unexpectedly exploded into this real-life thing that is now consuming all my open-source volunteerism, enthusiasm, and time. A man cannot have two mistresses. Either Gentoo has gotta go or @beartype has gotta go. Guess which is going?

C:DDA is this overlay's most popular ebuild, by a wide margin. It would be really sad for this overlay to just slowly die – and take Gentoo support for C:DDA with it. If you'd like to keep Gentoo support for C:DDA alive, please please consider adding C:DDA to Portage. You are the roguelike hero that Gentoo needs and deserves.

You can do this, @nethershaw. We all believe in you. You are the nail bat that the zombie hordes have been waiting for. 🧟

@leycec leycec merged commit 364890b into leycec:master May 28, 2024
@nethershaw
Copy link
Contributor Author

nethershaw commented May 28, 2024

I will do my best. I have already started working some other problems related to this ebuild that I think will improve its chances of being included in ::gentoo, namely:

  • The versioning scheme is ... yes, it's upstream's fault, but we can do better, and we can also support live release candidate ebuilds, if we think of upstream's use of alphabetic version components as a representation of the true semantic version in base-36. 0.G is actually 0.16; 0.H will be 0.17; et cetera. Release candidate live ebuilds from the upstream 0.H-branch ref will be 0.17.9999.
  • The upstream Makefile does some very very untoward things with compiler flags, especially with the Makefile flag LTO=1 set, which ::gentoo will definitely not approve of, and I need to find all those and clean them up.
  • Upstream still doesn't seem to want us using CMake. I could tackle converting the ebuild from autotools -> cmake, but I prefer to follow upstream guidance in these cases because that indicates they are still fiddling with it probably, so I think I'm going to balk on that for now and keep using autotools (I guess, unless I later discover there to be some obvious/significant advantage I'm currently missing).
  • Probably other things I haven't noticed yet and/or am forgetting right now.

@leycec
Copy link
Owner

leycec commented May 29, 2024

if we think of upstream's use of alphabetic version components as a representation of the true semantic version in base-36. 0.G is actually 0.16; 0.H will be 0.17; et cetera.

Base 36. I'll just let that sink in.

The upstream Makefile does some very very untoward things with compiler flags

🤮

so I think I'm going to balk on that for now and keep using autotools

Totally. And feel free to continue submitting C:DDA PRs against this repo, if you'd like. I'd also be happy to grant you push access as well.... if you'd like. No biggie either way. Clearly, you're both inherently trustworthy and awesome.

Thanks so much for tackling this whenever your scarce free time permits, @nethershaw. Now, if anyone needs me, I'll be sleeping in my bunk for a week. 😴

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.

2 participants