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

openra with extra mods #53300

Merged
merged 1 commit into from Jan 10, 2019

Conversation

Projects
None yet
5 participants
@msteen
Copy link
Contributor

msteen commented Jan 3, 2019

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
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@FRidh
Copy link
Member

FRidh left a comment

What was the motivation for opening a second PR?

Show resolved Hide resolved pkgs/top-level/all-packages.nix Outdated
@msteen

This comment has been minimized.

Copy link
Contributor Author

msteen commented Jan 3, 2019

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 openra package. I closed the previous PR.

@msteen msteen referenced this pull request Jan 3, 2019

Closed

init openra-mods #53163

4 of 10 tasks complete

@msteen msteen force-pushed the msteen:openra branch 2 times, most recently from a4eaa15 to 7e8f099 Jan 3, 2019

Show resolved Hide resolved pkgs/games/openra/builds.nix Outdated
Show resolved Hide resolved pkgs/games/openra/builds.nix Outdated
Show resolved Hide resolved pkgs/games/openra/builds.nix Outdated
Show resolved Hide resolved pkgs/games/openra/builds.nix Outdated

checkTarget = "test";

installPhase = ''

This comment has been minimized.

@veprbl

veprbl Jan 3, 2019

Member

Add runHook preInstall

This comment has been minimized.

@msteen

msteen Jan 3, 2019

Author Contributor

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.

This comment has been minimized.

@veprbl

veprbl Jan 8, 2019

Member

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.

This comment has been minimized.

@msteen

msteen Jan 8, 2019

Author Contributor

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.

This comment has been minimized.

@veprbl

veprbl Jan 9, 2019

Member

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).

This comment has been minimized.

@FRidh

FRidh Jan 9, 2019

Member

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?

Show resolved Hide resolved pkgs/games/openra/mods.nix Outdated
Show resolved Hide resolved pkgs/games/openra/builds.nix Outdated
Show resolved Hide resolved pkgs/games/openra/mod.nix
Show resolved Hide resolved pkgs/games/openra/mod.nix
Show resolved Hide resolved pkgs/games/openra/mod.nix Outdated

@msteen msteen force-pushed the msteen:openra branch from 7e8f099 to 296ec00 Jan 3, 2019

@fusion809

This comment has been minimized.

Copy link
Contributor

fusion809 commented Jan 5, 2019

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;
      };
    };
  };

@msteen msteen force-pushed the msteen:openra branch from 296ec00 to 0a6f827 Jan 5, 2019

@msteen

This comment has been minimized.

Copy link
Contributor Author

msteen commented Jan 5, 2019

I have added it to the mod list, thank you!

@FRidh
Copy link
Member

FRidh left a comment

How does one use the engines and mods?

Show resolved Hide resolved pkgs/games/openra/builds.nix Outdated
Show resolved Hide resolved pkgs/games/openra/engine.nix Outdated
Show resolved Hide resolved pkgs/top-level/all-packages.nix Outdated

@msteen msteen force-pushed the msteen:openra branch 2 times, most recently from 8d3821a to a62e50c Jan 5, 2019

@msteen

This comment has been minimized.

Copy link
Contributor Author

msteen commented Jan 5, 2019

How does one use the engines and mods?

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 nix-build '<nixpkgs>' --attr openraPackages.engines.stable provides ./result/bin/openra-{cnc,d2k,ra,ts}, and nix-build '<nixpkgs>' --attr openraPackages.mods.ss provides just ./result/bin/openra-ss

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.

@FRidh

This comment has been minimized.

Copy link
Member

FRidh commented Jan 5, 2019

Could you describe this in the main default.nix. Basically, give a brief overview of the various files, what they're for, and how one actually uses it.

@msteen

This comment has been minimized.

Copy link
Contributor Author

msteen commented Jan 5, 2019

There is no default.nix, but I guess I can place a comment, describing what they do, above common.nix, engine.nix, and mod.nix.

@msteen msteen force-pushed the msteen:openra branch 2 times, most recently from c80e7bc to 684493c Jan 5, 2019

@msteen

This comment has been minimized.

Copy link
Contributor Author

msteen commented Jan 5, 2019

@FRidh I have documented the files that could use some documentation.

@FRidh
Copy link
Member

FRidh left a comment

This looks good to me.

One more small suggestion. In the top of packages.nix, state that this file contains all openra packages, and that e.g. Sole Survivor is the attribute openraPackages.mods.ss.

Show resolved Hide resolved pkgs/games/openra/mods.nix Outdated
@FRidh

This comment has been minimized.

Copy link
Member

FRidh commented Jan 5, 2019

There is no default.nix

Maybe move packages.nix to default.nix then? It's typically the name of the first file one goes into.

@msteen msteen force-pushed the msteen:openra branch 2 times, most recently from fa8d7ce to 1567e55 Jan 5, 2019

@msteen

This comment has been minimized.

Copy link
Contributor Author

msteen commented Jan 5, 2019

I have implemented all of your feedback.

The packages.nix file has been renamed to default.nix, explaining that it defines all OpenRA packages, and I have added a bit of an overview to it.

I have added recurseIntoAttrs to the buildOpenRASet function, so both the engine and mod sets will have them set.

@FRidh

FRidh approved these changes Jan 5, 2019

Copy link
Member

FRidh left a comment

Thanks!

@msteen msteen force-pushed the msteen:openra branch from 1567e55 to bbc2394 Jan 6, 2019

Show resolved Hide resolved pkgs/games/openra/default.nix Outdated
@veprbl

This comment has been minimized.

Copy link
Member

veprbl commented Jan 9, 2019

This one fails to build for me:
@GrahamcOfBorg build openraPackages.mods.yr

@veprbl

This comment has been minimized.

Copy link
Member

veprbl commented Jan 9, 2019

This one too:
@GrahamcOfBorg build openraPackages.mods.rv

@veprbl

This comment has been minimized.

Copy link
Member

veprbl commented Jan 9, 2019

This too?
@GrahamcOfBorg build openraPackages.mods.d2

@veprbl

This comment has been minimized.

Copy link
Member

veprbl commented Jan 9, 2019

@GrahamcOfBorg openraPackages.mods.sp

@fusion809

This comment has been minimized.

Copy link
Contributor

fusion809 commented Jan 9, 2019

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 IBM.PAL, if you do not have the required game assets. If you do have the appropriate files in ~/.openra/Content/d2, on the other hand, it launches and runs fine.

@msteen msteen force-pushed the msteen:openra branch from 682a30b to 9a7997e Jan 9, 2019

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 ({

This comment has been minimized.

@veprbl

veprbl Jan 9, 2019

Member
Suggested change Beta
buildOpenRASet = f: args: pkgs.recurseIntoAttrs (builtins.mapAttrs callWithName (f ({
buildOpenRASet = f: args: pkgs.recurseIntoAttrs (stdenv.lib.mapAttrs callWithName (f ({

This comment has been minimized.

@msteen

msteen Jan 9, 2019

Author Contributor

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.

This comment has been minimized.

@veprbl

veprbl Jan 9, 2019

Member

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

Show resolved Hide resolved pkgs/games/openra/packages.nix
@veprbl

This comment has been minimized.

Copy link
Member

veprbl commented Jan 9, 2019

builtins.mapAttrs is too new yet. Let's use lib.mapAttrs, it uses builtins.mapAttrs only when it's available.

@msteen msteen force-pushed the msteen:openra branch 2 times, most recently from ddb9394 to 159d1bb Jan 9, 2019

@msteen

This comment has been minimized.

Copy link
Contributor Author

msteen commented Jan 9, 2019

I have build them all locally, I had to create a unsafeBuildOpenRAMod that disables checking for 4 mods that fail with errors when testing the maps, but seem to work without error when just playing the game, as as tested by @fusion809.

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.

@veprbl

This comment has been minimized.

Copy link
Member

veprbl commented Jan 9, 2019

@GrahamcOfBorg build openra
@GrahamcOfBorg build openraPackages.engines.bleed
@GrahamcOfBorg build openraPackages.engines.playtest
@GrahamcOfBorg build openraPackages.mods.ca
@GrahamcOfBorg build openraPackages.mods.d2
@GrahamcOfBorg build openraPackages.mods.dr
@GrahamcOfBorg build openraPackages.mods.gen
@GrahamcOfBorg build openraPackages.mods.kknd
@GrahamcOfBorg build openraPackages.mods.mw
@GrahamcOfBorg build openraPackages.mods.ra2
@GrahamcOfBorg build openraPackages.mods.raclassic
@GrahamcOfBorg build openraPackages.mods.rv
@GrahamcOfBorg build openraPackages.mods.sp
@GrahamcOfBorg build openraPackages.mods.ss
@GrahamcOfBorg build openraPackages.mods.ura
@GrahamcOfBorg build openraPackages.mods.yr

@msteen msteen force-pushed the msteen:openra branch from 159d1bb to 1ae7384 Jan 9, 2019

@msteen

This comment has been minimized.

Copy link
Contributor Author

msteen commented Jan 9, 2019

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.

@veprbl

veprbl approved these changes Jan 9, 2019

@veprbl veprbl merged commit 6bc8a66 into NixOS:master Jan 10, 2019

10 checks passed

grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details

fusion809 added a commit to fusion809/common-scripts that referenced this pull request Jan 11, 2019

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