Skip to content

wesnoth: fix darwin and LLVM 19 compatibility#381227

Merged
niklaskorz merged 5 commits intoNixOS:masterfrom
niklaskorz:wesnoth-llvm-19
May 5, 2025
Merged

wesnoth: fix darwin and LLVM 19 compatibility#381227
niklaskorz merged 5 commits intoNixOS:masterfrom
niklaskorz:wesnoth-llvm-19

Conversation

@niklaskorz
Copy link
Member

@niklaskorz niklaskorz commented Feb 11, 2025

LLVM 19 removed support for types not officially supported by std::char_traits: https://libcxx.llvm.org/ReleaseNotes/19.html#deprecations-and-removals
Wesnoth relies on this previously supported non-standard behavior for some types that should either have been a vector or span: wesnoth/wesnoth#9546
Unfortunately we can't just use std::span as that requires C++20, and making Wesnoth C++20 compatible is a non-trivial problem.
As a workaround, we make use of gsl-lite, which includes a lightweight implementation of span.

I've applied the relevant commits from 1.19 that wrap boost::span, and kept my own patch for fixing the lightmap (see comment above the patch).

Furthermore, while this package built on darwin before Clang 19 landed, it didn't run, as Wesnoth requires the binary to be contained in a macOS app bundle to function. This is also fixed in this PR by manually creating the app bundle using the upstream resources and Info.plist.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added the 6.topic: games Gaming on NixOS label Feb 11, 2025
@niklaskorz niklaskorz force-pushed the wesnoth-llvm-19 branch 2 times, most recently from c09cc3a to 501c0cd Compare February 11, 2025 17:12
@github-actions github-actions bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Feb 11, 2025
@niklaskorz niklaskorz changed the title wesnoth: fix LLVM 19 compatibility wesnoth: fix darwin and LLVM 19 compatibility Feb 12, 2025
@niklaskorz niklaskorz marked this pull request as ready for review February 12, 2025 00:43
@github-actions github-actions bot added 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. and removed 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Feb 12, 2025
@nix-owners nix-owners bot requested a review from abbradar February 12, 2025 00:51
@niklaskorz niklaskorz requested a review from a team February 13, 2025 00:03
@Samasaur1
Copy link
Member

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 381227


x86_64-darwin

✅ 1 package built:
  • wesnoth (wesnoth-dev)

aarch64-darwin

✅ 1 package built:
  • wesnoth (wesnoth-dev)

Copy link
Member

@Samasaur1 Samasaur1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I did notice that this won't support nix run, and I have seen some macOS app PRs recently that add a little shim in $out/bin that launches the app. Up to you whether you want to include one or not — I suspect that it's rare for anyone to try to nix run this package anyway

@emilazy
Copy link
Member

emilazy commented Feb 13, 2025

I’m a little worried about the patch that upstream presumably wouldn’t accept. Can we do something that doesn’t pull in another dependency?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is llvm-19 specific, can we do a more precise version check here? Since nixpkgs allows using both earlier and later versions, it may be nicer to only apply this for llvm19+

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough, will do!

@niklaskorz
Copy link
Member Author

I’m a little worried about the patch that upstream presumably wouldn’t accept. Can we do something that doesn’t pull in another dependency?

The upstream solution would be adopting C++20 imho, which is a bit beyond the scope of this PR. The only other alternative I see would be to vendor an even more lightweight implementation of span than gsl-lite, e.g. https://github.com/martinmoene/span-lite (which is also packaged in nixpkgs already), but I'm generally more confident in using gsl-lite as it's more widely used and more actively maintained.

Upstreaming this fix could use my changes as a starting point, but replacing gsl-lite with C++20's std::span and then going from there to fix all other incompatibilities with C++20.

@niklaskorz
Copy link
Member Author

Looks good to me. I did notice that this won't support nix run, and I have seen some macOS app PRs recently that add a little shim in $out/bin that launches the app. Up to you whether you want to include one or not — I suspect that it's rare for anyone to try to nix run this package anyway

That's a good idea, it might not be the most frequent way to access it but is nice for quickly trying out an app without installing it. I originally wanted to symlink the app's MacOS dir to bin, but that's not sufficient for the binary to locate the app bundle.

@azuwis
Copy link
Contributor

azuwis commented Feb 13, 2025

Something like this should make nix run works:

ln -s $out/Applications/Moonlight.app/Contents/MacOS/Moonlight $out/bin/moonlight

@Samasaur1
Copy link
Member

Looks good to me. I did notice that this won't support nix run, and I have seen some macOS app PRs recently that add a little shim in $out/bin that launches the app. Up to you whether you want to include one or not — I suspect that it's rare for anyone to try to nix run this package anyway

That's a good idea, it might not be the most frequent way to access it but is nice for quickly trying out an app without installing it. I originally wanted to symlink the app's MacOS dir to bin, but that's not sufficient for the binary to locate the app bundle.

I think the approach that would work is a script that runs open -na "$(realpath "$out/Applications/The Battle for Wesnoth.app")"

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one person. label Feb 14, 2025
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Mar 16, 2025
@niklaskorz
Copy link
Member Author

Rebasing and applying wesnoth/wesnoth#10102 right now

@ofborg ofborg bot added the 6.topic: darwin Running or building packages on Darwin label Apr 27, 2025
@niklaskorz niklaskorz removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Apr 27, 2025
@niklaskorz
Copy link
Member Author

Looks good to me. I did notice that this won't support nix run, and I have seen some macOS app PRs recently that add a little shim in $out/bin that launches the app. Up to you whether you want to include one or not — I suspect that it's rare for anyone to try to nix run this package anyway

That's a good idea, it might not be the most frequent way to access it but is nice for quickly trying out an app without installing it. I originally wanted to symlink the app's MacOS dir to bin, but that's not sufficient for the binary to locate the app bundle.

I think the approach that would work is a script that runs open -na "$(realpath "$out/Applications/The Battle for Wesnoth.app")"

Added, but without the realpath as the app bundle itself is no symlink

@niklaskorz
Copy link
Member Author

I’m a little worried about the patch that upstream presumably wouldn’t accept. Can we do something that doesn’t pull in another dependency?

The additional dependency has been removed as upstream now automatically uses either std::span (for C++20), boost::span (for boost >= 1.78) or a vendored fallback (not relevant for us as we provide boost 1.86 to wesnoth). And before you ask... yeah, I could've used boost::span in my original patch instead of gsl-lite but wasn't aware that boost ships an implementation of span now.

When 1.20 is out, the patches can be removed entirely as upstream's 1.19 branch already includes all required fixes.

@niklaskorz
Copy link
Member Author

Rebased because of the tree-wide removal of obsolete darwin frameworks in #398707

@niklaskorz niklaskorz merged commit de8b819 into NixOS:master May 5, 2025
32 of 35 checks passed
@niklaskorz niklaskorz deleted the wesnoth-llvm-19 branch May 26, 2025 07:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: darwin Running or building packages on Darwin 6.topic: games Gaming on NixOS 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 12.approvals: 1 This PR was reviewed and approved by one person.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants