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

Jump to bottom

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

Merged
merged 1 commit into from Dec 11, 2019
Merged

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

merged 1 commit into from Dec 11, 2019

Conversation

shazow
Copy link
Contributor

@shazow shazow commented Nov 24, 2019 •
edited

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 November 24, 2019 18:57
Br1ght0ne
Br1ght0ne approved these changes Nov 24, 2019
View reviewed changes
Copy link
Member

@Br1ght0ne Br1ght0ne left a comment

Choose a reason for hiding this comment

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

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

@mnacamura
Copy link
Contributor

@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 commented Nov 26, 2019 via email

@shazow
Copy link
Contributor Author

shazow commented Dec 1, 2019 •
edited

@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 commented Dec 1, 2019

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

@mnacamura
Copy link
Contributor

mnacamura commented Dec 1, 2019 •
edited

@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

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

@shazow
Copy link
Contributor Author

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
Member

@GrahamcOfBorg build cataclysm-dda-git

@shazow
Copy link
Contributor Author

shazow commented Dec 1, 2019

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

@Br1ght0ne
Copy link
Member

@GrahamcOfBorg build cataclysm-dda-git

@mnacamura
Copy link
Contributor

Works for me on Darwin. Thanks.

@shazow
Copy link
Contributor Author

shazow commented Dec 2, 2019

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

@shazow
Copy link
Contributor Author

shazow commented Dec 5, 2019

Ping. 🔔

@shazow
Copy link
Contributor Author

shazow commented Dec 11, 2019

Another friendly ping.

@Br1ght0ne
Copy link
Member

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
@marsam
Copy link
Contributor

marsam commented Dec 11, 2019

LGTM, thanks!

@ghost ghost mentioned this pull request Aug 12, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mnacamura mnacamura mnacamura approved these changes

@Br1ght0ne Br1ght0ne Br1ght0ne approved these changes

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

Successfully merging this pull request may close these issues.

None yet
4 participants
@shazow @mnacamura @Br1ght0ne @marsam