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.

Sign up for GitHub

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

Jump to bottom

openarena: add community map pack #84868

Closed
wants to merge 1 commit into from
Closed

openarena: add community map pack #84868

wants to merge 1 commit into from

Conversation

tomfitzhenry
Copy link
Contributor

@tomfitzhenry tomfitzhenry commented Apr 10, 2020 •
edited

Motivation for this change

More maps in Openarena!

To test, install openarena and start a game using one of the community maps from https://openarena.fandom.com/wiki/OACMP/Volume_1 , e.g. oacmpdm1.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@tomfitzhenry tomfitzhenry changed the title :openarena: add community map pack openarena: add community map pack Apr 10, 2020
@tomfitzhenry
Copy link
Contributor Author

tomfitzhenry commented Apr 10, 2020 •
edited

ideally users of this package could choose whether they want the community maps.

The map pack needs to go into openarena's baseoa directory.

What's a good pattern to support optionality here?

Should the map pack be a separate derivation, and then copied into the openarena derivation? I think that'd make the openarena derivation large, and thus lose nix cache benefits. I couldn't find a way to configure openarena to look at multiple directories for game resources.

@progval
Copy link
Member

progval commented Apr 12, 2020 •
edited

The moddb.com link is dead.

Currently this one works, but it's probably temporary: https://sjc3.dl.dbolical.com/dl/2014/01/20/oacmp-volume1-v3.zip?st=ju5icWRS1x_v2PxfPD270A==&e=1586721284

@tomfitzhenry
Copy link
Contributor Author

The moddb.com link is dead.

@progval Ah, I feared that would happen, hence why I uploaded the zip to archive.org and included that URL too. I've updated the PR to only include the archive.org link. Please could you download the zip from the original site, and confirm the sha256 that I've put in the PR?

tomfitzhenry
tomfitzhenry commented Apr 19, 2020
View reviewed changes
pkgs/games/openarena/default.nix
@@ -31,6 +38,9 @@ stdenv.mkDerivation {
makeWrapper "${gameDir}/openarena.${arch}" "$out/bin/openarena" \
--prefix LD_LIBRARY_PATH : "${libPath}"
makeWrapper "${gameDir}/oa_ded.${arch}" "$out/bin/oa_ded"

cd openarena-0.8.8
unzip $communityMaps
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: Delete sources/ which is ~70% of the unzipped space.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

Choose a reason for hiding this comment

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

I think this fits better into the unpackPhase.

@tomfitzhenry
openarena: add community map pack
8d7d936 
The download links on https://openarena.fandom.com/wiki/OACMP/Volume_1
don't have stable URLs, so I have uploaded the zip file to archive.org.
@progval
Copy link
Member

progval commented Apr 19, 2020

Indeed:

val@odroidn2-02:~$ nix-prefetch-url https://www.moddb.com/downloads/mirror/64571/125/e71d69605cc137112f067d3503a6fdbd
[84.1 MiB DL]
path is '/nix/store/qyblwia1kpqvy2k9vjhr3yamya4rggk8-e71d69605cc137112f067d3503a6fdbd'
06z4cxp6mrcdxx3lra0v5n295a8mz6q13n173jcdlqyd1zgmqwvv

tomfitzhenry
tomfitzhenry commented Apr 21, 2020
View reviewed changes
pkgs/games/openarena/default.nix
@@ -10,6 +10,13 @@ stdenv.mkDerivation {
sha256 = "0jmc1cmdz1rcvqc9ilzib1kilpwap6v0d331l6q53wsibdzsz3ss";
};

# https://openarena.fandom.com/wiki/OACMP/Volume_1
communityMaps = fetchurl {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should this be an optional part of the openarena package?

Copy link
Contributor

Choose a reason for hiding this comment

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

It should definitely be optional but I think it can be enabled by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know how to make it optional. openarena.withMaps [ communityMaps ] would be a nice pattern for this?

Copy link
Contributor

Choose a reason for hiding this comment

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

You could have a communityMaps ? true as parameter and use lib.optionalString communityMaps "unzip $communityMaps" I think (would be a good idea to also make the fetchurl step optional), but maybe it would be better to convert the maps to another derivation and add a parameter withMaps like you said (then, you can use propagatedBuildInputs = withMaps to include the files from withMaps in the openarena).

Would be cleaner and more flexible that way.

@tomfitzhenry tomfitzhenry requested a review from kevincox May 3, 2020 02:56
@kevincox
Copy link
Contributor

kevincox commented May 6, 2020

Is it possibly to have the maps in a different package such that they can be referenced by a configuration file or similar? This way the openarena package doesn't depend on the maps.

@stale
Copy link

stale bot commented Nov 3, 2020

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Nov 3, 2020
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Nov 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thiagokokada thiagokokada thiagokokada left review comments

@SuperSandro2000 SuperSandro2000 SuperSandro2000 left review comments

@kevincox kevincox Awaiting requested review from kevincox

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 10.rebuild-linux: 1-10 10.rebuild-linux: 1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@tomfitzhenry @progval @kevincox @thiagokokada @SuperSandro2000