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

xu4: init at 1.3 #181866

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

xu4: init at 1.3 #181866

wants to merge 2 commits into from

Conversation

mausch
Copy link
Member

@mausch mausch commented Jul 17, 2022

Description of changes

http://xu4.sourceforge.net/
XU4 is a remake of the computer game Ultima IV.

It requires the original game files, which are not open source and is available from GOG and some other sites. The zip file then needs to be placed in ~/.local/share/xu4.

I'll probably make another PR with a separate unfree package getting that and some graphics upgrade patches so users don't need to set things up manually.

cc @WickedSmoke

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 22.11 Release Notes (or backporting 22.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@WickedSmoke
Copy link

Are you still going to package v1.0 now that 1.1 is available? There are some fixes that users should have.

@mausch
Copy link
Member Author

mausch commented Jul 17, 2022

Are you still going to package v1.0 now that 1.1 is available? There are some fixes that users should have.

It'd take me some time to adjust things to the changes in 1.1 and I've been using 1.0 with no issues for some time now...
Eventually we'll upgrade but for now I think it's better than not having the package at all, plus someone else might come along and make the upgrade.

pkgs/games/xu4/default.nix Outdated Show resolved Hide resolved
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/1016/37

@WickedSmoke
Copy link

Please note that this package is now three versions behind the latest xu4 1.3 release.

Also, libxml2 is not required if the tlkconv utility is not being built.

@mausch
Copy link
Member Author

mausch commented Sep 3, 2023

I just tried updating this.

v1.1+ adds a dependency on https://github.com/WickedSmoke/faun which isn't currently available on nixpkgs so that would have to be packaged first.

v1.2+ seems to get some git submodule during the build. This isn't allowed in Nix as it strictly separates the phase of getting sources from building things. @WickedSmoke can you shed some light into what this gets exactly, why it was necessary, maybe there's some way to work around it? Many thanks.

@WickedSmoke
Copy link

The GLV submodule is a new backend which is the default on Linux that replaces Allegro. This code is included in the source tarball so I recommend you use that rather than Git. The dist/xu4.spec file for RPM has been updated and may be the best example of how your could package for your distribution.

It's still possible to use Allegro for display & audio, but the official builds use GLV & Faun so that's what I would recommend.

@WickedSmoke
Copy link

Faun also has an RPM dist/faun.spec file you can study.

Also note that there are five new game & music modules that you could provide as a package (see https://xu4.sourceforge.net/download.php#release).

@mausch
Copy link
Member Author

mausch commented Oct 1, 2023

@WickedSmoke I managed to get 1.1 to build with faun but it crashes at startup with:
xu4: error: Unable to obtain OpenGL resource (gui.png)
Apparently related to xu4-engine/u4@df18271 ?

@WickedSmoke
Copy link

If the gui.png is missing from render.pak then you need to rebuild that file. Perhaps the 1.0 version already exists?

Could you try building the 1.3 version first? I'd like to avoid spending time dealing with outdated ones.

@mausch mausch changed the title xu4: init at 1.0 xu4: init at 1.1 Oct 1, 2023
@mausch mausch changed the title xu4: init at 1.1 xu4: init at 1.3 Oct 4, 2023

nativeBuildInputs = [ makeWrapper ];

# there's no `install` target

Choose a reason for hiding this comment

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

A make install target does exist. It installs the dist/xu4.desktop item though, which for some reason is ignored and remade below.

Also, why does the binary get copied as $out/share/xu4?

Copy link
Member Author

Choose a reason for hiding this comment

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

A make install target does exist.

Ah I see it's been added in 1.1 👍 xu4-engine/u4@bb02ea4
However it includes u4upgrade.zip and I haven't been able to find its license so I'm packaging that separately ( #185063 ).

It installs the dist/xu4.desktop item though, which for some reason is ignored and remade below.

Using your desktop file now as of 571e028 👍

Also, why does the binary get copied as $out/share/xu4?

This is so that bin is clean and only contains a single binary (a small wrapper, it's pretty common in Nix) and not the modules and other files.

@mausch
Copy link
Member Author

mausch commented Oct 14, 2023

@WickedSmoke Thanks for the reviews! Could you approve the PR to improve the chances of getting merged?

@WickedSmoke
Copy link

I don't think libXxf86vm is required, so you should try to build without that. Once you've checked that I'll be happy to approve the PR.

@mausch
Copy link
Member Author

mausch commented Oct 15, 2023

I don't think libXxf86vm is required, so you should try to build without that.

Done in 2dbdf0b , it works 👍

Copy link

@WickedSmoke WickedSmoke left a comment

Choose a reason for hiding this comment

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

From the game side of things this looks good. Seeing as Mauricio started the packaging process over a year ago I hope the PR is approved soon.

Note that version 1.4 is likely to be released within a month, so you should upgrade ASAP after that. I don't expect any changes for the packaging so a simple version bump should do the trick.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/1187

];

configurePhase = ''
patchShebangs .
Copy link
Member

Choose a reason for hiding this comment

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

This should be done in postPatch or preConfigure to use the standard configurPhase

Copy link
Member Author

@mausch mausch Oct 31, 2023

Choose a reason for hiding this comment

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

Unfortunately the standard configurePhase won't work here because it runs ./configure --prefix=$out which this nonstandard configure doesn't like, it wants ./configure --prefix $out (no equals) 🤷‍♂️
I'll add a comment about this.

Copy link
Member

Choose a reason for hiding this comment

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

Do we just need to to patchShebang the configure script or anything else? It is always nice if we can avoid the . for all files.


postConfigure = ''
# need to report this upstream
sed --in-place 's/-Wall/-Wno-error=format-security/g' src/Makefile
Copy link
Member

Choose a reason for hiding this comment

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

Can we disable werror completely?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see Werror set anywhere in the repo 🤷‍♂️

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, that's strange

nativeBuildInputs = [ makeWrapper ];

# The `install` target references some files with unknown license
installPhase =
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
installPhase =
installPhase = ''

Copy link
Member Author

Choose a reason for hiding this comment

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

Applied in 467e54a

Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

other than that one comment LGTM

];

configurePhase = ''
patchShebangs .
Copy link
Member

Choose a reason for hiding this comment

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

Do we just need to to patchShebang the configure script or anything else? It is always nice if we can avoid the . for all files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants