dwarf-fortress: address code review feedback

pkgs/games/dwarf-fortress/default.nix

@@ -38,15 +38,17 @@ let
     getAttr
     importJSON
     listToAttrs
+    optionalAttrs
     recurseIntoAttrs
     replaceStrings
+    versionAtLeast
     ;
 
   callPackage = newScope self;
 
   # The latest Dwarf Fortress version. Maintainers: when a new version comes
   # out, ensure that (unfuck|dfhack|twbt) are all up to date before changing
-  # this. Note that unfuck and twbt are not required for v50.
+  # this. Note that unfuck and twbt are not required for 50.
   latestVersion = "50.12";
 
   # Converts a version to a package name.
@@ -58,16 +60,16 @@ let
       name = versionToName dfVersion;
       value =
         let
-          isV50 = lib.versionAtLeast dfVersion "50.0";
+          isAtLeast50 = versionAtLeast dfVersion "50.0";
 
-          dwarf-fortress-unfuck = if isV50 then null else callPackage ./unfuck.nix { inherit dfVersion; };
+          dwarf-fortress-unfuck = optionalAttrs (!isAtLeast50) (callPackage ./unfuck.nix { inherit dfVersion; });
 
           dwarf-fortress = callPackage ./game.nix {
             inherit dfVersion;
             inherit dwarf-fortress-unfuck;
           };
 
-          twbt = if isV50 then null else callPackage ./twbt { inherit dfVersion; };
+          twbt = optionalAttrs (!isAtLeast50) (callPackage ./twbt { inherit dfVersion; });
 
           dfhack = callPackage ./dfhack {
             inherit (perlPackages) XMLLibXML XMLLibXSLT;
@@ -77,10 +79,13 @@ let
 
           dwarf-therapist = libsForQt5.callPackage ./dwarf-therapist/wrapper.nix {
             inherit dwarf-fortress;
-            dwarf-therapist = libsForQt5.callPackage ./dwarf-therapist {
+            dwarf-therapist = (libsForQt5.callPackage ./dwarf-therapist {
               texlive = texliveBasic.withPackages (ps: with ps; [ float caption wrapfig adjmulticol sidecap preprint enumitem ]);
-              inherit isV50;
+            }).override (optionalAttrs (!isAtLeast50) {
-            };
+              # 41.2.5 is the last version to support Dwarf Fortress 0.47.
+              version = "41.2.5";
+              hash = "sha256-xfYBtnO1n6OcliVt07GsQ9alDJIfWdVhtuyWwuvXSZs=";
+            });
           };
         in
         callPackage ./wrapper {

pkgs/games/dwarf-fortress/dfhack/default.nix

@@ -84,18 +84,6 @@ let
       xmlRev = "cca87907c1cbfcf4af957b0bea3a961a345b1581";
       needsPatches = true;
     };
-    "50.10" = {
-      dfHackRelease = "50.10-r1.1";
-      hash = "sha256-k2j8G4kJ/RYE8W0YDOxcsRb5qjjn4El+rigf0v3AqZU=";
-      xmlRev = "041493b221e0799c106abeac1f86df4535ab80d3";
-      prerelease = false;
-    };
-    "50.11" = {
-      dfHackRelease = "50.11-r7";
-      hash = "sha256-3KsFc0i4XkzoeRvcl5GUlx/fJB1HyqfZm+xL6T4oT/A=";
-      xmlRev = "cca87907c1cbfcf4af957b0bea3a961a345b1581";
-      prerelease = false;
-    };
     "50.12" = {
       dfHackRelease = "50.12-r3rc1";
       hash = "sha256-EcM/FLulGVJgaERFMpYi9O5i1QKZyFb0X4HQagVnO8k=";
@@ -112,8 +100,8 @@ let
     else throw "[DFHack] Unsupported Dwarf Fortress version: ${dfVersion}";
 
   version = release.dfHackRelease;
-  isV50 = versionAtLeast version "50.0";
+  isAtLeast50 = versionAtLeast version "50.0";
-  needsV50Patches = isV50 && (release.needsPatches or false);
+  needs50Patches = isAtLeast50 && (release.needsPatches or false);
 
   # revision of library/xml submodule
   xmlRev = release.xmlRev;
@@ -167,11 +155,11 @@ in
       name = "fix-protobuf.patch";
       url = "https://github.com/DFHack/dfhack/commit/7bdf958518d2892ee89a7173224a069c4a2190d8.patch";
       hash = "sha256-p+mKhmYbnhWKNiGPMjbYO505Gcg634n0nudqH0NX3KY=";
-    }) ++ optional needsV50Patches (fetchpatch {
+    }) ++ optional needs50Patches (fetchpatch {
       name = "use-system-sdl2.patch";
       url = "https://github.com/DFHack/dfhack/commit/734fb730d72e53ebe67f4a041a24dd7c50307ee3.patch";
       hash = "sha256-uLX0gdVSzKEVibyUc1UxcQzdYkRm6D8DF+1eSOxM+qU=";
-    }) ++ optional needsV50Patches (fetchpatch {
+    }) ++ optional needs50Patches (fetchpatch {
       name = "rename-lerp.patch";
       url = "https://github.com/DFHack/dfhack/commit/389dcf5cfcdb8bfb8deeb05fa5756c9f4f5709d1.patch";
       hash = "sha256-QuDtGURhP+nM+x+8GIKO5LrMcmBkl9JSHHIeqzqGIPQ=";
@@ -196,8 +184,8 @@ in
 
     # We don't use system libraries because dfhack needs old C++ ABI.
     buildInputs = [ zlib ]
-      ++ optional isV50 SDL2
+      ++ optional isAtLeast50 SDL2
-      ++ optional (!isV50) SDL
+      ++ optional (!isAtLeast50) SDL
       ++ optionals enableStoneSense [ allegro5 libGLU libGL ];
 
     preConfigure = ''

pkgs/games/dwarf-fortress/dwarf-therapist/default.nix

@@ -6,23 +6,20 @@
 , cmake
 , texlive
 , ninja
-, isV50 ? true
+, version ? "42.1.5"
+, hash ? "sha256-aUakfUjnIZWNDhCkG3A6u7BaaCG8kPMV/Fu2S73CoDg="
 }:
 
 stdenv.mkDerivation rec {
   pname = "dwarf-therapist";
 
-  # 41.2.5 is the last version to support Dwarf Fortress 0.47.
+  inherit version;
-  version = if isV50 then "42.1.5" else "41.2.5";
 
   src = fetchFromGitHub {
     owner = "Dwarf-Therapist";
     repo = "Dwarf-Therapist";
     rev = "v${version}";
-    hash = if isV50 then # latest
+    inherit hash;
-      "sha256-aUakfUjnIZWNDhCkG3A6u7BaaCG8kPMV/Fu2S73CoDg="
-    else # 41.2.5
-      "sha256-xfYBtnO1n6OcliVt07GsQ9alDJIfWdVhtuyWwuvXSZs=";
   };
 
   nativeBuildInputs = [ texlive cmake ninja ];

pkgs/games/dwarf-fortress/game.nix

@@ -26,7 +26,6 @@ let
     hasAttr
     licenses
     maintainers
-    makeLibraryPath
     optional
     optionals
     optionalString
@@ -53,14 +52,8 @@ let
   baseVersion = toInt (elemAt dfVersionTuple dfVersionBaseIndex);
   patchVersion = elemAt dfVersionTuple (dfVersionBaseIndex + 1);
 
-  isV50 = baseVersion >= 50;
+  isAtLeast50 = baseVersion >= 50;
-  enableUnfuck = !isV50 && dwarf-fortress-unfuck != null;
+  enableUnfuck = !isAtLeast50 && dwarf-fortress-unfuck != null;
-
-  libpath = makeLibraryPath (
-    [ stdenv.cc.cc stdenv.cc.libc ]
-    ++ optional (!isV50) SDL
-    ++ optional enableUnfuck dwarf-fortress-unfuck
-  );
 
   game =
     if hasAttr dfVersion df-hashes
@@ -92,17 +85,21 @@ stdenv.mkDerivation {
   sourceRoot = ".";
 
   postUnpack = optionalString stdenv.isLinux ''
-    if [ -d df_linux ]; then
+    directory=${
-      mv df_linux/* .
+      if stdenv.isLinux then "df_linux"
-    fi
+      else if stdenv.isDarwin then "df_osx"
-  '' + optionalString stdenv.isDarwin ''
+      else throw "Unsupported system"
-    if [ -d df_osx ]; then
+    }
-      mv df_osx/* .
+    if [ -d "$directory" ]; then
+      mv "$directory/"* .
     fi
   '';
 
-  nativeBuildInputs = optional isV50 autoPatchelfHook;
+  nativeBuildInputs = [ autoPatchelfHook ];
-  buildInputs = optionals isV50 [ SDL2 SDL2_image SDL2_mixer stdenv.cc.cc.lib ];
+  buildInputs = optionals isAtLeast50 [ SDL2 SDL2_image SDL2_mixer ]
+    ++ optional (!isAtLeast50) SDL
+    ++ optional enableUnfuck dwarf-fortress-unfuck
+    ++ [ stdenv.cc.cc.lib ];
 
   installPhase = ''
     runHook preInstall
@@ -118,15 +115,12 @@ stdenv.mkDerivation {
     [ -f $out/df ] && chmod +x $out/df
     [ -f $out/run_df ] && chmod +x $out/run_df
 
+    # We don't need any of these since they will just break autoPatchelf on <version 50.
+    [ -d $out/libs ] && rm -f $out/libs/*.so $out/libs/*.so.*
+
     # Store the original hash
     md5sum $exe | awk '{ print $1 }' > $out/hash.md5.orig
     echo "Original MD5: $(<$out/hash.md5.orig)" >&2
-  '' + optionalString (!isV50 && stdenv.isLinux) ''
-    rm -f $out/libs/*.so
-    patchelf \
-      --set-interpreter $(cat ${stdenv.cc}/nix-support/dynamic-linker) \
-      --set-rpath "${libpath}" \
-      $exe
   '' + optionalString stdenv.isDarwin ''
     # My custom unfucked dwarfort.exe for macOS. Can't use
     # absolute paths because original doesn't have enough
@@ -148,14 +142,16 @@ stdenv.mkDerivation {
     runHook postInstall
   '';
 
-  fixupPhase = ''
+  preFixup = ''
-    runHook preFixup
+    recompute_hash() {
-    runHook postFixup
-
       # Store the new hash as the very last step.
       exe=$out/${exe}
       md5sum $exe | awk '{ print $1 }' > $out/hash.md5
       echo "Patched MD5: $(<$out/hash.md5)" >&2
+    }
+
+    # Ensure that this runs after autoPatchelfHook.
+    trap recompute_hash EXIT
   '';
 
   passthru = {

pkgs/games/dwarf-fortress/twbt/default.nix

@@ -81,8 +81,12 @@ stdenvNoCC.mkDerivation rec {
     cp -a *.png $art/data/art/
   '';
 
+  passthru = {
+    inherit dfVersion;
+  };
+
   meta = {
-    description = "A plugin for Dwarf Fortress / DFHack that improves various aspects the game interface";
+    description = "A plugin for Dwarf Fortress / DFHack that improves various aspects of the game interface";
     maintainers = with maintainers; [ Baughn numinit ];
     license = licenses.mit;
     platforms = platforms.linux;

pkgs/games/dwarf-fortress/wrapper/default.nix

@@ -39,9 +39,10 @@ let
     inherit enableStoneSense;
   };
 
-  isV50 = dwarf-fortress.baseVersion >= 50;
+  isAtLeast50 = dwarf-fortress.baseVersion >= 50;
 
-  enableTWBT' = enableTWBT && (twbt != null);
+  # If TWBT is null or the dfVersion is wrong, it isn't supported (for example, on version 50).
+  enableTWBT' = enableTWBT && twbt != null && (twbt.dfVersion or null) == dwarf-fortress.version;
 
   ptheme =
     if builtins.isString theme
@@ -200,27 +201,31 @@ stdenv.mkDerivation rec {
   nativeInstallCheckInputs = [ expect xvfb-run ];
 
   installCheckPhase = let
-    commonExpectComponents = fmod: ''
+    commonExpectStatements = fmod: lib.optionalString isAtLeast50 ''
-      ${lib.optionalString isV50 ''expect "Loading audio..."''}
+      expect "Loading audio..."
-      ${lib.optionalString (!fmod && isV50) ''expect "Failed to load fmod, trying SDL_mixer"''}
+    '' + lib.optionalString (!fmod && isAtLeast50) ''
-      ${lib.optionalString isV50 ''expect "Audio loaded successfully!"''}
+      expect "Failed to load fmod, trying SDL_mixer"
+    '' + lib.optionalString isAtLeast50 ''
+      expect "Audio loaded successfully!"
+    '' + ''
       expect "Loading bindings from data/init/interface.txt"
     '';
-    dfHackExpectScript = writeText "dfhack-test.exp" ''
+    dfHackExpectScript = writeText "dfhack-test.exp" (''
-      spawn xvfb-run $env(out)/bin/dfhack
+      spawn env NIXPKGS_DF_OPTS=debug xvfb-run $env(out)/bin/dfhack
-      ${commonExpectComponents false}
+    '' + commonExpectStatements false + ''
+      expect "DFHack is ready. Have a nice day!"
       expect "DFHack version ${version}"
       expect "\[DFHack\]#"
       send -- "lua print(os.getenv('out'))\r"
       expect "$env(out)"
       # Don't send 'die' here; just exit. Some versions of dfhack crash on exit.
       exit 0
-    '';
+    '');
-    vanillaExpectScript = fmod: writeText "vanilla-test.exp" ''
+    vanillaExpectScript = fmod: writeText "vanilla-test.exp" (''
-      spawn ${lib.optionalString fmod "env NIXPKGS_DF_OPTS=fmod"} xvfb-run $env(out)/bin/dwarf-fortress
+      spawn env NIXPKGS_DF_OPTS=debug,${lib.optionalString fmod "fmod"} xvfb-run $env(out)/bin/dwarf-fortress
-      ${commonExpectComponents fmod}
+    '' + commonExpectStatements fmod + ''
       exit 0
-    '';
+    '');
   in
   ''
     export HOME="$(mktemp -dt dwarf-fortress.XXXXXX)"
@@ -228,7 +233,7 @@ stdenv.mkDerivation rec {
     expect ${dfHackExpectScript}
     df_home="$(find ~ -name "df_*" | head -n1)"
     test -f "$df_home/dfhack"
-  '' + lib.optionalString isV50 ''
+  '' + lib.optionalString isAtLeast50 ''
     expect ${vanillaExpectScript true}
     df_home="$(find ~ -name "df_*" | head -n1)"
     test ! -f "$df_home/dfhack"

pkgs/games/dwarf-fortress/wrapper/dwarf-fortress-init.in

@@ -1,64 +1,136 @@
 #!@stdenv_shell@ -e
+set -euo pipefail
 shopt -s extglob
 
-env_dir="@env@"
+export NIXPKGS_DF_ENV="@env@"
 
-if [ -n "$DF_DIR" ]; then
+if [[ -v DF_DIR ]] && [ -n "$DF_DIR" ] && { [[ ! -v NIXPKGS_DF_HOME ]] || [ -z "$NIXPKGS_DF_HOME" ]; }; then
   # Compatibility for users that were using DF_DIR, since the dfhack script clobbers this variable.
   export NIXPKGS_DF_HOME="$DF_DIR"
 fi
 
-if [ -z "$NIXPKGS_DF_HOME" ]; then
+if [[ ! -v NIXPKGS_DF_HOME ]] || [ -z "$NIXPKGS_DF_HOME" ]; then
   export NIXPKGS_DF_HOME="${XDG_DATA_HOME:-$HOME/.local/share}/df_linux"
 fi
 
 # Compatibility.
 export DF_DIR="$NIXPKGS_DF_HOME"
 
-update_path() {
+### BEGIN: Default DF options
-  local path="$1"
-
-  @mkdir@ -p "$NIXPKGS_DF_HOME/$(dirname "$path")"
-
-  # If user has replaced these data directories, let them stay.
-  if [ ! -e "$NIXPKGS_DF_HOME/$path" ] || [ -L "$NIXPKGS_DF_HOME/$path" ]; then
-    @rm@ -f "$NIXPKGS_DF_HOME/$path"
-    @ln@ -s "$env_dir/$path" "$NIXPKGS_DF_HOME/$path"
-  fi
-}
-
-cleanup_path() {
-  local path="$1"
-
-  # Let them stay if not a link.
-  if [ ! -e "$NIXPKGS_DF_HOME/$path" ] || [ -L "$NIXPKGS_DF_HOME/$path" ]; then
-    @rm@ -f "$NIXPKGS_DF_HOME/$path"
-  fi
-}
-
-forcecopy_path() {
-  local path="$1"
-
-  @mkdir@ -p "$NIXPKGS_DF_HOME/$(dirname "$path")"
-  @rm@ -rf "$NIXPKGS_DF_HOME/$path"
-  @cp@ -rL --no-preserve=all "$env_dir/$path" "$NIXPKGS_DF_HOME/$path"
-}
-
 declare -A _NIXPKGS_DF_OPTS
+_NIXPKGS_DF_OPTS[fmod]=0    # Don't use fmod by default.
+_NIXPKGS_DF_OPTS[debug]=0   # No debugging output by default.
+### END: Default DF options
 
-# Don't use fmod by default.
+# Read NIXPKGS_DF_OPTS.
-_NIXPKGS_DF_OPTS[fmod]=0
+if [[ ! -v NIXPKGS_DF_OPTS ]]; then
-
+  NIXPKGS_DF_OPTS=''
-IFS=',' read -ra split_options <<< "$NIXPKGS_DF_OPTS"
+fi
-for option in "${split_options[@]}"; do
+IFS=',' read -ra options <<< "$NIXPKGS_DF_OPTS"
+for option in ${options[@]+"${options[@]}"}; do
   key="${option%=*}"
   value="${option##*=}"
+  if [ -n "$key" ]; then
     if [ -z "$value" ] || [ "$key" == "$value" ]; then
       value=1
     fi
     _NIXPKGS_DF_OPTS["$key"]="$value"
+  fi
 done
 
+# Rebuild the canonical option string from the read options.
+NIXPKGS_DF_OPTS=''
+for key in "${!_NIXPKGS_DF_OPTS[@]}"; do
+  value="${_NIXPKGS_DF_OPTS["${key}"]}"
+  NIXPKGS_DF_OPTS="$NIXPKGS_DF_OPTS$key=$value,"
+done
+NIXPKGS_DF_OPTS="${NIXPKGS_DF_OPTS%,}"
+
+# Echoes a log.
+# $@: log messages
+log() {
+  for msg in "$@"; do
+    echo "[nixpkgs] $msg" >&2
+  done
+}
+
+# Echoes a log if NIXPKGS_DF_OPTS includes debug.
+# $@: log messages
+debug() {
+  if [ "${_NIXPKGS_DF_OPTS[debug]}" -ne 0 ]; then
+    log "$@"
+  fi
+}
+
+# Updates a path in $NIXPKGS_DF_HOME from $NIXPKGS_DF_ENV.
+# $1: The environment path.
+update_path() {
+  local path="$1"
+  local orig="$NIXPKGS_DF_ENV/$path"
+  local final="$NIXPKGS_DF_HOME/$path"
+
+  # If user has replaced these data directories, let them stay.
+  @mkdir@ -p "$(dirname -- "$final")"
+  if [ ! -e "$final" ] || [ -L "$final" ]; then
+    debug "Linking: $final -> $orig"
+    @rm@ -f "$final"
+    @ln@ -s "$orig" "$final"
+  else
+    debug "Not updating: $final"
+  fi
+}
+
+# Cleans up a path in $NIXPKGS_DF_HOME that may or may not be in $NIXPKGS_DF_ENV.
+# $1: The environment path.
+cleanup_path() {
+  local path="$1"
+  local final="$NIXPKGS_DF_HOME/$path"
+
+  # Let them stay if not a link.
+  if [ ! -e "$final" ] || [ -L "$final" ]; then
+    debug "Cleaning up: $final"
+    @rm@ -f "$final"
+  else
+    debug "Not cleaning: $final"
+  fi
+}
+
+# Force copies a path in $NIXPKGS_DF_HOME that may or may not be in $NIXPKGS_DF_ENV.
+# $1: The environment path.
+forcecopy_path() {
+  local path="$1"
+
+  if [ -z "$NIXPKGS_DF_ENV" ] || [ -z "$path" ]; then
+    # Avoid producing "/" for any `rm -rf`
+    return
+  fi
+
+  local orig="$NIXPKGS_DF_ENV/$path"
+  local final="$NIXPKGS_DF_HOME/$path"
+
+  if [ -e "$orig" ]; then
+    debug "Force copying: $orig -> $final"
+    @mkdir@ -p "$(dirname -- "$final")"
+    @rm@ -rf "$final"
+    @cp@ -rL --no-preserve=all "$orig" "$final"
+  else
+    debug "Removing: $final"
+    @rm@ -rf "$final"
+  fi
+}
+
+# Runs the final executable. Expects NIXPKGS_DF_HOME and NIXPKGS_DF_EXE to be set.
+go() {
+  cd "$NIXPKGS_DF_HOME"
+  debug "Executing: $NIXPKGS_DF_HOME/$NIXPKGS_DF_EXE"
+
+  # If we make it past here, we want to log.
+  # shellcheck disable=SC2093
+  exec -a "$NIXPKGS_DF_EXE" "$NIXPKGS_DF_HOME/$NIXPKGS_DF_EXE"
+  log "Execution of $NIXPKGS_DF_HOME/$NIXPKGS_DF_EXE failed!"
+  exit 1
+}
+
 @mkdir@ -p "$NIXPKGS_DF_HOME"
 
 @cat@ <<EOF >&2
@@ -75,13 +147,13 @@ done
 | pull request if there are any that become a problem.                         |
 |                                                                              |
 | We started with the following nixpkgs launch options as NIXPKGS_DF_OPTS:     |
-| $(@printf@ '% -76s' "$(IFS=',' echo "${_NIXPKGS_DF_OPTS[@]@K}")") |
+| $(@printf@ '% -76s' "$NIXPKGS_DF_OPTS") |
 |                                                                              |
 | If you want to try fmod over SDL_mixer, set NIXPKGS_DF_OPTS=fmod.            |
 \\------------------------------------------------------------------------------/
 EOF
 
-cd "$env_dir"
+cd "$NIXPKGS_DF_ENV"
 
 # All potential important files in DF 50 and below.
 for path in dwarfort *.so libs raw data/init/* data/!(init|index|announcement); do
@@ -99,15 +171,11 @@ done
 
 # These need to be copied due to read only flags on older versions of DF.
 for path in index announcement help dipscript; do
-  if [ -e "data/$path" ]; then
   forcecopy_path "data/$path"
-  else
-    @rm@ -f "$NIXPKGS_DF_HOME/$path" &>/dev/null
-  fi
 done
 
 # Handle library paths on Darwin.
 if [ "$(@uname@)" == Darwin ]; then
-  export DYLD_LIBRARY_PATH="$env_dir/libs"
+  export DYLD_LIBRARY_PATH="$NIXPKGS_DF_ENV/libs"
-  export DYLD_FRAMEWORK_PATH="$env_dir/libs"
+  export DYLD_FRAMEWORK_PATH="$NIXPKGS_DF_ENV/libs"
 fi

pkgs/games/dwarf-fortress/wrapper/dwarf-fortress.in

@@ -1,11 +1,8 @@
 #!@stdenv_shell@ -e
 
+export NIXPKGS_DF_EXE="@dfExe@"
 source @dfInit@
 
-set -euo pipefail
-
-exe="@dfExe@"
-
 # If we're switching back from dfhack to vanilla, cleanup all dfhack
 # links so Dwarf Fortress doesn't autoload its leftover libdfhooks.so.
 # Otherwise, populate them.
@@ -21,7 +18,7 @@ dfhack_files=(
   *.init *.init-example
 )
 
-if [ "${exe##*/}" == dfhack ]; then
+if [ "${NIXPKGS_DF_EXE##*/}" == dfhack ]; then
   for i in "${dfhack_files[@]}"; do
     if [ -e "$i" ]; then
       update_path "$i"
@@ -35,5 +32,4 @@ else
   done
 fi
 
-# Go!
+go
-cd "$NIXPKGS_DF_HOME" && exec "./$exe" "$@"

pkgs/games/dwarf-fortress/wrapper/soundSense.in

@@ -1,10 +1,10 @@
 #!@stdenv_shell@ -e
 
+export NIXPKGS_DF_EXE="soundsense/soundSense.sh"
 source @dfInit@
 
-for p in soundsense/*; do
+for path in soundsense/*; do
-  update_path "$p"
+  update_path "$path"
 done
 
-cd "$DF_DIR"
+PATH="@jre@/bin:$PATH" go
-PATH=@jre@/bin exec $DF_DIR/soundsense/soundSense.sh