openra with extra mods #53300
openra with extra mods #53300
Conversation
|
The reason for a new PR is that I am no longer just adding packages, but in this PR I am also replacing the existing |
|
|
||
| checkTarget = "test"; | ||
|
|
||
| installPhase = '' |
There was a problem hiding this comment.
Hmm, I have mixed feelings about this. It is not hard to work around not having these hooks present if you wanted to override something of the package and not all phases/hooks have to be available for a package, like when the phases attribute is used.
There was a problem hiding this comment.
Well, the problem is that, if someone (maybe even future you) tries to override preInstall, it will not work without any warning. Nobody wants to have that kind of moment, so I think it would be better if you put those runHooks in.
There was a problem hiding this comment.
I am still not convinced, because you will have to check the package definition anyway to make sure you are not overwriting any existing preInstall. And since only few packages add those runHook calls, it is probably better to not assume adding such a phase will just work, instead it is best to always check the package definition. Is there some guideline stating they should be added? Any other arguments to change my mind? I would love to know, so far I have asked twice on the IRC, but gotten no response. However since you seem to feel strongly about it and it does not have any other effect on the build, I have added them where possible.
There was a problem hiding this comment.
A naive override of a phase without using runHook calls breaks pre- and post- hooks. This is not a consistent interface and can lead to unpleasant user experience. We could say that this is a bug in stdenv and we have to work around it. There is some work on improving this treewide: https://github.com/dezgeg/rfcs/blob/phase-changes/rfcs/0032-run-phase-changes-for-better-nix-shell-use.md
AFAIK there is no guideline for applying this workaround and you are right to point out the lack of consistency in its use across nixpkgs. And I do appreciate that it might make no sense for you to add it to your expression. I think, the only way to really resolve this would be to fix stdenv (e.g by means of RFC above).
There was a problem hiding this comment.
We expect functions like buildPythonPackage, which may override phases, to ensure hooks are still available, but not every individual derivation. It's also difficult to do; what if the derivation implements a postInstall, should it then, when explicitly passed in a postInstall, override it, or extend it?
|
One worthy addition to this PR is the Sole Survivor mod: ss = buildOpenRAMod {
version = "72";
title = "Sole Survivor";
description = "A re-imagination of the original Command & Conquer: Sole Survivor game";
homepage = https://github.com/MustaphaTR/sole-survivor;
src = fetchFromGitHub {
owner = "MustaphaTR";
repo = "sole-survivor";
rev = "fad65579c8b487cef9a8145e872390ed77c16c69";
sha256 = "0h7is7x2qyvq7vqp0jgw5zrdkw8g7ndd82d843ldhnb0a3vyrk34";
};
engine = rec {
version = "becfc15";
src = fetchFromGitHub {
owner = "OpenRA";
repo = "OpenRA" ;
rev = version;
sha256 = "0id8vf3cjr7h5pz4sw8pdaz3sc45lxr21k1fk4309kixsrpa7i0y";
name = "engine";
inherit extraPostFetch;
};
};
}; |
|
I have added it to the mod list, thank you! |
Both engines and mods provide you with OpenRA mods to play. The difference is that an engine provides several mods as part of the engine, and out-of-tree (or out-of-engine if you will) mods provide only that particular mod. For example Even though I do not know of any engine forks that provide other mods as part of the engine, it was easy to define engines like mods in a generic way and it would for example now also allow for a bleeding edge version of the OpenRA engine to exist. The out-of-tree mods require the source code of the engine as part of their building process. And the engine needed is always some specific version of OpenRA or some fork of it. |
|
Could you describe this in the main |
|
There is no |
|
@FRidh I have documented the files that could use some documentation. |
Maybe move |
|
I have implemented all of your feedback. The I have added |
GrahamcOfBorg
added
8.has: package (new)
10.rebuild-linux: 11-100
and removed
10.rebuild-linux: 1-10
labels
|
This one fails to build for me: |
|
This one too: |
|
This too? |
|
@GrahamcOfBorg openraPackages.mods.sp |
|
I think we can ignore them, because all four of the mods with test failures you flagged, run fine for me, after being built without tests. I've started single player campaigns, created a few units, infiltrated some buildings with my engineers, collected some resources, etc., all without problem, in my efforts to see if they work. Beware, if you try testing them yourself, D2 crashes on start up, with an error message complaining of a missing file called |
pkgs/games/openra/default.nix
Outdated
| if the attribute name and engine/mod name are equal. | ||
| */ | ||
| callWithName = name: value: if builtins.isFunction value then value name else value; | ||
| buildOpenRASet = f: args: pkgs.recurseIntoAttrs (builtins.mapAttrs callWithName (f ({ |
There was a problem hiding this comment.
| buildOpenRASet = f: args: pkgs.recurseIntoAttrs (builtins.mapAttrs callWithName (f ({ | |
| buildOpenRASet = f: args: pkgs.recurseIntoAttrs (stdenv.lib.mapAttrs callWithName (f ({ |
There was a problem hiding this comment.
Though it should be safe for usage in 18.09 and up, right? Anyway, I have changed it to use lib instead as you suggested.
There was a problem hiding this comment.
Yes. Judging by release notes you are right about NixOS version. However, nixpkgs must evaluate on Nix as old as 2.0 as stated by current https://github.com/NixOS/nixpkgs/blob/master/lib/minver.nix
|
|
|
I have build them all locally, I had to create a Some mods need unfree assets to be installed, so instead of letting them crash with an error message or letting them ask to look for the installation media of the original games, I have added an error message stating that they need those assets with a link for more information. |
|
@GrahamcOfBorg build openra |
|
I forgot to push changing the name/version of the engines. I thought it better to move the engine name part in the version, since whether it is release, playtest or bleed is saying something about the OpenRA version, not the name. @veprbl I am sorry this push undid the results of the requested builds, but I checked before pushing and they all passed except for the Darwin builds, which where neutral since they did not apply. |
Motivation for this change
This updates the existing OpenRA package with extra mods (thanks to @fusion809) and tries to share as much as possible between the code necessary to package an engine or an out-of-tree mod. This PR incorporates the feedback given on the previous PR (#53163) and replaces it.
cc: @veprbl @FRidh
Things done
sandboxinnix.confon non-NixOS)nix-shell -p nox --run "nox-review wip"./result/bin/)nix path-info -Sbefore and after)