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: init at 0.1.1 #30409

Merged
merged 1 commit into from Oct 18, 2017
Merged

openrct2: init at 0.1.1 #30409

merged 1 commit into from Oct 18, 2017

Conversation

oxzi
Copy link
Member

@oxzi oxzi commented Oct 14, 2017

Motivation for this change

This PR adds the open source re-implementation of RollerCoaster Tycoon 2, OpenRCT2, to the nixpkgs.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-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/)
  • Fits CONTRIBUTING.md.

@oxzi
Copy link
Member Author

oxzi commented Oct 14, 2017 •
edited

I just updated the expression for darwin to add AppKit which failed before. If this doesn't work, I'd like to hear some suggestions from a Mac-user.

Edit: Adding darwin.apple_sdk.frameworks.AppKit as a buildInput for darwin haven't changed a thing. The error is still the same: fatal error: module 'AppKit' not found

Edit 2: I've restricted platforms = platforms.linux because I don't exactly know what the problem with MacOS is and there aren't much hits for AppKit in the nixpkgs. If some Mac-user wants to work on this, I'm happy to welcome your changes.

fpletz
fpletz approved these changes Oct 14, 2017
View reviewed changes
Copy link
Member

@fpletz fpletz left a comment

Choose a reason for hiding this comment

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

Thanks! Works like a charm on my NixOS machine.

@vcunat vcunat mentioned this pull request Oct 15, 2017
Closed
7 tasks
@vcunat
Copy link
Member

vcunat commented Oct 15, 2017

Just for record, there's also an older PR: #16685

@oxzi
Copy link
Member Author

oxzi commented Oct 15, 2017 •
edited

@vcunat Sorry, I didn't knew about the older one and, for some reasons, haven't searched for it. However, the mentioned PR seems to be stalled and is outdated. Are there arguments against proceeding with this one?

@vcunat
Copy link
Member

vcunat commented Oct 15, 2017

I know of no such arguments. I mainly wanted to notify both sides.

Mic92
Mic92 reviewed Oct 15, 2017
View reviewed changes
pkgs/games/openrct2/default.nix Outdated
cd ${sourceRoot}/build
cmake -DCMAKE_BUILD_TYPE=RELWITHDEBINFO \
-DDOWNLOAD_TITLE_SEQUENCES=OFF \
-DCMAKE_INSTALL_PREFIX=$out \
Copy link
Member

Choose a reason for hiding this comment

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

cmakeFlags = []; can be used here.

Copy link
Member Author

Choose a reason for hiding this comment

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

It seems like one can't write $out in cmakeFlags. The workarounds I found in the nixpkgs where working with adding this cmake-flag as a command in preConfigure which seems quite ugly for me.

Copy link
Member

@Mic92 Mic92 Oct 16, 2017 •
edited

Choose a reason for hiding this comment

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

you don't need to, it is already added by default: https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/tools/build-managers/cmake/setup-hook.sh#L29

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I didn't knew how smart the cmake-wrapper was. Thanks!

Mic92
Mic92 reviewed Oct 15, 2017
View reviewed changes
pkgs/games/openrct2/default.nix Outdated

configurePhase = ''
mkdir ${sourceRoot}/build
mkdir ${sourceRoot}/usr
Copy link
Member

Choose a reason for hiding this comment

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

this could go into preConfigure then build should be handled by cmake itself.

Mic92
Mic92 reviewed Oct 15, 2017
View reviewed changes
pkgs/games/openrct2/default.nix Outdated

buildPhase = "make all g2";

installPhase = "make install";
Copy link
Member

Choose a reason for hiding this comment

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

this is the default.

Mic92
Mic92 reviewed Oct 15, 2017
View reviewed changes
pkgs/games/openrct2/default.nix Outdated
..
'';

buildPhase = "make all g2";
Copy link
Member

Choose a reason for hiding this comment

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

makeFlags = ["all" "g2"];

@oxzi oxzi force-pushed the openrct2 branch 3 times, most recently from 3ef1e68 to 9f91719 Compare October 16, 2017 18:50
@oxzi
Copy link
Member Author

oxzi commented Oct 18, 2017

Are there any more wishes for changes? How's the status?

@Mic92 Mic92 merged commit 30c046b into NixOS:master Oct 18, 2017
@oxzi oxzi deleted the openrct2 branch October 18, 2017 13:54
@janisozaur
Copy link
Contributor

Nice to see this merged! You may want to update the list here: https://github.com/OpenRCT2/OpenRCT2#2-downloading-the-game-pre-built

The other PR, #16685, had some innoextract-based package to automatically fetch the assets from https://gog.com, any chance of reviving this?

@oxzi
Copy link
Member Author

oxzi commented Oct 21, 2017

I'll create a PR to add the NixOS-package to this list.

However, I have no knowledge of GOG and honestly don't want to use it. If someone has interest in this, perhaps a new issue or - even better - a PR should be created.

@oxzi oxzi mentioned this pull request Oct 21, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mic92 Mic92 Mic92 left review comments

@fpletz fpletz fpletz approved these changes

Assignees
No one assigned
Labels
8.has: package (new)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@oxzi @vcunat @janisozaur @Mic92 @fpletz