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

cataclysm-dda-git: 2019-05-03 -> 2019-11-22 #74064

Merged
merged 1 commit into from Dec 11, 2019
Merged

Conversation

@shazow
Copy link
Contributor

@shazow shazow commented Nov 24, 2019

Motivation for this change

The cataclysm-dda-git package is pinned to a specific version, and the previous version was bumped more than 6 months ago. There has been some substantial work done on the game, so it's good to keep it bumped occasionally.

I built the newest version without the locale patch (since it didn't apply cleanly) and it built/ran fine. I'm not sure if there was a reason why it was needed originally that I missed (didn't find any relevant comments in the old commits), so I removed it for now.

Played the game for a while, seems to work great.

If it is needed, here's a new backported verison of the patch that we can add back in:

diff --git a/src/translations.cpp b/src/translations.cpp
index 2bd330314e..c786216930 100644
--- a/src/translations.cpp
+++ b/src/translations.cpp
@@ -211,14 +211,12 @@ void set_language()
     auto env = getenv( "LANGUAGE" );
     locale_dir = std::string( PATH_INFO::base_path() + "lang/mo/" + ( env ? env : "none" ) +
                               "/LC_MESSAGES/cataclysm-dda.mo" );
-#elif (defined(__linux__) || (defined(MACOSX) && !defined(TILES)))
+#else
     if( !PATH_INFO::base_path().empty() ) {
         locale_dir = PATH_INFO::base_path() + "share/locale";
     } else {
         locale_dir = "lang/mo";
     }
-#else
-    locale_dir = "lang/mo";
 #endif

     const char *locale_dir_char = locale_dir.c_str();
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 nix-review --run "nix-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.
Notify maintainers

cc @mnacamura @rardiol

@shazow shazow changed the title cataclysm-dda-git: version bump cataclysm-dda-git: 2019-05-03 -> 2019-11-22 Nov 24, 2019
@ofborg ofborg bot requested a review from mnacamura Nov 24, 2019
Copy link
Contributor

@Br1ght0ne Br1ght0ne left a comment

Checked with nix-review on NixOS x86_64. Works fine.

@mnacamura
Copy link
Contributor

@mnacamura mnacamura commented Nov 26, 2019

@shazow Did you test console build with translations? Without the deleted patch, it should be broken as far as I remember.

@shazow
Copy link
Contributor Author

@shazow shazow commented Nov 26, 2019

@shazow
Copy link
Contributor Author

@shazow shazow commented Dec 1, 2019

@mnacamura I tried changing the language with the tiles build, it works fine.

Haven't had a chance to try it without tiles yet, got nix-env -f . -iE 'p: (p {}).cataclysm-dda-git.override { tiles = false; }' queued up, is that what you mean regarding the "console build"?

@shazow
Copy link
Contributor Author

@shazow shazow commented Dec 1, 2019

Confirmed that no tiles with non-English language also works. Anything else needs checking?

@mnacamura
Copy link
Contributor

@mnacamura mnacamura commented Dec 1, 2019

@shazow Thank you for checking and sorry my memory was incorrect.
I remember now (and have confirmed on Darwin) that the patch fixes the path to translation files for the tiles build on macOS. Could you attach the updated patch shown below to your PL?

diff --git a/src/translations.cpp b/src/translations.cpp
index 067e2cd77d..5660d18b3d 100644
--- a/src/translations.cpp
+++ b/src/translations.cpp
@@ -211,14 +211,12 @@ void set_language()
     auto env = getenv( "LANGUAGE" );
     locale_dir = std::string( FILENAMES["base_path"] + "lang/mo/" + ( env ? env : "none" ) +
                               "/LC_MESSAGES/cataclysm-dda.mo" );
-#elif (defined(__linux__) || (defined(MACOSX) && !defined(TILES)))
+#else
     if( !FILENAMES["base_path"].empty() ) {
         locale_dir = FILENAMES["base_path"] + "share/locale";
     } else {
         locale_dir = "lang/mo";
     }
-#else
-    locale_dir = "lang/mo";
 #endif

     const char *locale_dir_char = locale_dir.c_str();
@mnacamura
Copy link
Contributor

@mnacamura mnacamura commented Dec 1, 2019

@shazow Ah you've already prepared the backport patch. Thanks.

@shazow shazow force-pushed the shazow:cdda-git-bump branch from ce79fd4 to dc2c221 Dec 1, 2019
@shazow
Copy link
Contributor Author

@shazow shazow commented Dec 1, 2019

@mnacamura Force-pushed the change, can you confirm it works for you on Darwin now? I don't have access to a Darwin machine. :) Thanks!

@Br1ght0ne
Copy link
Contributor

@Br1ght0ne Br1ght0ne commented Dec 1, 2019

@GrahamcOfBorg build cataclysm-dda-git

@shazow shazow force-pushed the shazow:cdda-git-bump branch from dc2c221 to 5e8770a Dec 1, 2019
@shazow
Copy link
Contributor Author

@shazow shazow commented Dec 1, 2019

Looks like my patch wasn't applying cleanly, switched to using your patch.

@Br1ght0ne
Copy link
Contributor

@Br1ght0ne Br1ght0ne commented Dec 1, 2019

@GrahamcOfBorg build cataclysm-dda-git

@mnacamura
Copy link
Contributor

@mnacamura mnacamura commented Dec 2, 2019

Works for me on Darwin. Thanks.

@shazow
Copy link
Contributor Author

@shazow shazow commented Dec 2, 2019

Great. :) Do we need to do anything else before merging?

@shazow
Copy link
Contributor Author

@shazow shazow commented Dec 5, 2019

Ping. 🔔

@shazow
Copy link
Contributor Author

@shazow shazow commented Dec 11, 2019

Another friendly ping.

@Br1ght0ne
Copy link
Contributor

@Br1ght0ne Br1ght0ne commented Dec 11, 2019

I think this is ready to be merged.
@marsam can you help out? Thanks!

@marsam marsam merged commit 98e57f8 into NixOS:master Dec 11, 2019
16 checks passed
16 checks passed
cataclysm-dda-git on x86_64-linux Timed out, unknown build status
Details
Evaluation Performance Report Evaluator Performance Report
Details
cataclysm-dda-git on aarch64-linux Success
Details
cataclysm-dda-git on x86_64-darwin Success
Details
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-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
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
@marsam
Copy link
Contributor

@marsam marsam commented Dec 11, 2019

LGTM, thanks!

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

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.