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

tic-80: init at 1.1.2837 #291129

Merged
merged 1 commit into from
Apr 1, 2024
Merged

tic-80: init at 1.1.2837 #291129

merged 1 commit into from
Apr 1, 2024

Conversation

blinry
Copy link
Contributor

@blinry blinry commented Feb 24, 2024

Description of changes

TIC-80 is a "fantasy console" for making, playing and sharing tiny games. See the packaging request #200260.

This is based on @winny-'s private package, which I updated to the latest release, and their kind permission to take it and contribute it to nixpkgs.

This is not a particularly elegant package, because TIC-80 vendors a lot of its dependencies. Especially the SDL2 and mruby dependencies require workarounds. In the long term, maybe we could upstream options to TIC-80's CMake build process to use Nix' libraries instead of building them from scratch. But for now, I like this how it is.

How to try out this package

If you have enabled Flakes, you simply can try running the following command:

nix run github:blinry/nixpkgs/tic-80#tic-80

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@blinry
Copy link
Contributor Author

blinry commented Feb 24, 2024

@winny- I'd love to add you as a co-maintainer! Would you like that?

I didn't see you in nixpkgs' maintainer file yet, so if you'd want details like email address, github handle/id or other contact options listed there, let me know! (But all of these are optional.)

@blinry
Copy link
Contributor Author

blinry commented Feb 24, 2024

Also pinging @pinkcreeper100, @CyborgPotato and @Serif-7, maybe you'd like to help review or test? :)

@pinkcreeper100
Copy link
Contributor

tried to run this and got a mismatched hash

warning: Ignoring setting 'auto-allocate-uids' because experimental feature 'auto-allocate-uids' is not enabled
warning: Ignoring setting 'impure-env' because experimental feature 'configurable-impure-env' is not enabled
error: hash mismatch in fixed-output derivation '/nix/store/0ym3nzcv368j4yfv4r7d28f6540hrfyq-source.drv':
         specified: sha256-2hSaLWw57F19tSfgJvBgbqW52vCu8p/TmoybYzQEybE=
            got:    sha256-8aChH1YHb04tqRzsV1UGaIhUsUAPEN+/x9TrVbhUy5M=
error: 1 dependencies of derivation '/nix/store/dfi6c61iv9pqs08zgmw747b85ff1idlk-tic-80-1.1.2837.drv' failed to build

@blinry
Copy link
Contributor Author

blinry commented Feb 24, 2024

@pinkcreeper100 Thanks for testing! Ah dang! That might be #8567 at work…

Could you check whether /nix/store/bsrazmw8j1yd4h0vnqkx3a7pbcfr2i7x-source (the output path of the unhappy source derivation) is still created, and if so, make a zip file of it, and upload it somewhere? That way, I could compare it to my copy, and check what goes wrong.

Alternatively, we could fall back to @winny-'s original solution to just hardcode the revision number in the CMake build process...

@pinkcreeper100
Copy link
Contributor

the directory doesn't exist

pkgs/by-name/ti/tic-80/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ti/tic-80/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ti/tic-80/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ti/tic-80/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ti/tic-80/package.nix Outdated Show resolved Hide resolved

cmakeFlags = [ "-DBUILD_PRO=On" "-DBUILD_SDLGPU=On" ];
enableParallelBuilding = true;
dontStrip = true;
Copy link
Member

Choose a reason for hiding this comment

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

we should add a short reason with why

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My thought was that it'd be required for the rpath patching. But I tried without it, and it didn't seem to make a difference? So I guess we can remove it until we find it's required.

pkgs/by-name/ti/tic-80/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ti/tic-80/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ti/tic-80/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ti/tic-80/package.nix Outdated Show resolved Hide resolved
@blinry
Copy link
Contributor Author

blinry commented Feb 24, 2024

Thanks for your detailed review, @SuperSandro2000! 🎉 I addressed all your points in a follow-up commit (which we could squash once we merge this PR).

All except one: Using the vendored libraries seems much less complicated right now compared to re-modelling the compilation process to allow using Nix' versions.

@winny-
Copy link
Contributor

winny- commented Feb 24, 2024

@winny- I'd love to add you as a co-maintainer! Would you like that?

Hi there, thanks for asking! I'm unable to support nixpkgs work at this time.

@blinry
Copy link
Contributor Author

blinry commented Feb 25, 2024

I removed the requirement to use leaveDotGit, and went with @winny-'s suggestion not to build the pro version by default. Happy with this PR, I think!

@winny-
Copy link
Contributor

winny- commented Mar 2, 2024

@SuperSandro2000 hi, are there any additional review items?

Copy link
Member

@Atemu Atemu left a comment

Choose a reason for hiding this comment

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

A couple minor things and questions, other than that, diff LGTM

pkgs/by-name/ti/tic-80/package.nix Show resolved Hide resolved
pkgs/by-name/ti/tic-80/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ti/tic-80/package.nix Show resolved Hide resolved
@blinry
Copy link
Contributor Author

blinry commented Mar 31, 2024

Thanks for the review, @Atemu! I addressed your suggestions in f488870.

@Atemu
Copy link
Member

Atemu commented Mar 31, 2024

Could you squash the fixups into the original commits?

@Atemu
Copy link
Member

Atemu commented Mar 31, 2024

Result of nixpkgs-review pr 291129 run on x86_64-linux 1

1 package built:
  • tic-80

I don't have any games to test but it appears to run as expected. With a delightful startup sound too :)

@blinry
Copy link
Contributor Author

blinry commented Mar 31, 2024

Could you squash the fixups into the original commits?

Done :)

(Sorry for the notification, @mmahut & @RaghavSood, your review is not required.)

src = fetchFromGitHub {
owner = "nesbox";
repo = "TIC-80";
rev = "v" + version;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: String interpolation is usually preferred to concatenation.

@Atemu Atemu merged commit f2acaf2 into NixOS:master Apr 1, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants