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.

Sign up for GitHub

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

openrct2: 0.2.3 -> 0.2.4 #72212

Merged
merged 1 commit into from
Nov 14, 2019
Merged

openrct2: 0.2.3 -> 0.2.4 #72212

merged 1 commit into from
Nov 14, 2019
+11 −8

Conversation

lightbulbjim
Copy link
Contributor

Motivation for this change

Updating to latest version.

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 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 @geistesk

Sorry, something went wrong.

@ofborg ofborg bot requested a review from oxzi October 29, 2019 04:55
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Oct 29, 2019
jonringer
jonringer approved these changes Oct 29, 2019
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment •
edited
Loading

Choose a reason for hiding this comment

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

nix-review passes on NixOS
diff LGTM
executables seem to work
leaf package

https://github.com/NixOS/nixpkgs/pull/72212
1 package were build:
openrct2

@jonringer
Copy link
Contributor

@GrahamcOfBorg build openrct2

c0bw3b
c0bw3b reviewed Oct 29, 2019
View reviewed changes
Copy link
Contributor

@c0bw3b c0bw3b left a comment

Choose a reason for hiding this comment

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

cmake and pkgconfig should rather be in nativeBuildInputs

@lightbulbjim
Copy link
Contributor Author

cmake and pkgconfig should rather be in nativeBuildInputs

Fair enough. Updated.

oxzi
oxzi suggested changes Oct 30, 2019
View reviewed changes
Copy link
Member

@oxzi oxzi left a comment

Choose a reason for hiding this comment

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

Thanks for your PR. Please check the upstream CMakeLists.txt for updates, as already commented.

pkgs/games/openrct2/default.nix
rev = "v1.0.11";
sha256 = "1bh7mngpqnhzwnhhawq5y3a6hbvwxis2yagk4dcmc4w1fifq2y66";
rev = "v1.0.12";
sha256 = "0vfhyldc8nfvkg4d9kry669haxz2165walbxzgza7pqpnd7aqgrf";
};

Copy link
Member

Choose a reason for hiding this comment

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

The 0.2.4-release introduced a replay module in its CMakeLists.txt. This sub-project should be added like the objects and title sequences.

Copy link
Contributor Author

@lightbulbjim lightbulbjim Oct 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

replays is only used for tests, which we don't currently run.

If we want to run tests then we'll need to include googletest in the derivation. I'm not sure if there's a quick and easy way to disable the download, so we might need to patch test/tests/CMakeLists.txt as well.

Thoughts?

@ofborg ofborg bot requested a review from oxzi October 30, 2019 20:33
@lightbulbjim
Copy link
Contributor Author

Is there anything I can do to assist moving this forward?

@c0bw3b
Copy link
Contributor

c0bw3b commented Nov 13, 2019

If we stick to not running tests, could you add an explicite cmake flag with a comment :

  cmakeFlags = [
    "-DCMAKE_BUILD_TYPE=RELWITHDEBINFO"
    "-DDOWNLOAD_OBJECTS=OFF"
    "-DDOWNLOAD_TITLE_SEQUENCES=OFF"
    "-DWITH_TESTS=OFF" # requires GTest and fetching replays
  ];

OTOH you should be able to use gtest from nixpkgs by passing another cmake flag -DSYSTEM_GTEST (look at ceph package for example) and fetch replays like other modules.
But you would need to check locally if some other problems arise by running tests, the time it takes, etc.

@lightbulbjim
Copy link
Contributor Author

If we stick to not running tests, could you add an explicite cmake flag with a comment :

  cmakeFlags = [
    "-DCMAKE_BUILD_TYPE=RELWITHDEBINFO"
    "-DDOWNLOAD_OBJECTS=OFF"
    "-DDOWNLOAD_TITLE_SEQUENCES=OFF"
    "-DWITH_TESTS=OFF" # requires GTest and fetching replays
  ];

OTOH you should be able to use gtest from nixpkgs by passing another cmake flag -DSYSTEM_GTEST (look at ceph package for example) and fetch replays like other modules.
But you would need to check locally if some other problems arise by running tests, the time it takes, etc.

Sure, I'm happy to try and add the tests if that's what we want to do. My intuition would have been to leave the tests out since they weren't included in the derivation previously. I guess ultimately the decision is up to @geistesk as the maintainer.

oxzi
oxzi approved these changes Nov 14, 2019
View reviewed changes
Copy link
Member

@oxzi oxzi left a comment

Choose a reason for hiding this comment

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

Excuse my late reply. I have tested the submitted PR successfully.

Regarding the test discussion, I would like to note that by default WITH_TESTS is OFF. So I do not think this cmake flag needs to be added additionally. If you want to add the tests, I am certainly not stopping you. Anyway, the PR looks good for me in the current state.

jonringer
jonringer approved these changes Nov 14, 2019
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

nix-review passes on NixOS
diff LGTM
leaf package

[5 built, 5 copied (2.6 MiB), 0.3 MiB DL]
https://github.com/NixOS/nixpkgs/pull/72212
1 package were build:
openrct2

@jonringer
Copy link
Contributor

@GrahamcOfBorg build openrct2

@c0bw3b c0bw3b merged commit e8de40d into NixOS:master Nov 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonringer jonringer

@oxzi oxzi

@c0bw3b c0bw3b

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 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
@lightbulbjim @jonringer @c0bw3b @oxzi