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

ruffle: nightly-2024-11-07 -> nightly-2025-01-04 #371012

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

normalcea
Copy link

@normalcea normalcea commented Jan 5, 2025

Updates the Ruffle package to version nightly-2025-01-04, updating the relevant src, Cargo.lock and git hashes. Additionally, there is a patch for adding desktop file, icon and meta info support for Ruffle on Linux which installs them in the postInstall phase.

I can also confirm that #285887 is still an issue due to the deterministic build flag. To solve this, I believe we should have a ruffle-bin package that downloads a pre-compiled ruffle binary (which only contains the standalone player) which avoids the uphill task of separating derivations. It would also allow for easy nix-update-script usage to automatically update ruffle.

Issue #285887 can be reproduced using the various date swf files listed on this page: (https://edmullen.net/fclock.php).
which provides the dummy time (2001-02-03 04:05:06) when a Flash application requests the time.

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/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 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.

@github-actions github-actions bot added the 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` label Jan 5, 2025
@normalcea normalcea changed the title ruffle: nightly-2024-11-07 -> nightly-2025-01-04. ruffle: nightly-2024-11-07 -> nightly-2025-01-04 Jan 5, 2025
@nix-owners nix-owners bot requested review from GovanifY and jchv January 5, 2025 00:39
@normalcea normalcea force-pushed the update-ruffle branch 2 times, most recently from e058c96 to 4f0a3e7 Compare January 5, 2025 03:50
@lucasew
Copy link
Contributor

lucasew commented Jan 6, 2025

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 371012


x86_64-linux

✅ 1 package built:
  • ruffle

@jchv
Copy link
Contributor

jchv commented Jan 12, 2025

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 371012

@jchv
Copy link
Contributor

jchv commented Jan 12, 2025

For some reason nixpkgs-review is acting strange on my macOS box, and not actually do the build... but I tested on both NixOS and macOS and it seems to build and run.

@wegank wegank added 12.approvals: 1 This PR was reviewed and approved by one reputable person 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package labels Jan 12, 2025
@@ -97,9 +102,9 @@ rustPlatform.buildRustPackage {
cargoLock = {
lockFile = ./Cargo.lock;
Copy link
Member

Choose a reason for hiding this comment

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

Could use the new fetcher to avoid copying Cargo.lock into nixpkgs.

diff --git a/pkgs/by-name/ru/ruffle/package.nix b/pkgs/by-name/ru/ruffle/package.nix
index 5ac9463f0344..1e4bc1af870d 100644
--- a/pkgs/by-name/ru/ruffle/package.nix
+++ b/pkgs/by-name/ru/ruffle/package.nix
@@ -99,16 +99,8 @@ rustPlatform.buildRustPackage {
 
   cargoBuildFlags = [ "--workspace" ];
 
-  cargoLock = {
-    lockFile = ./Cargo.lock;
-    outputHashes = {
-      "flash-lso-0.6.0" = "sha256-LMfJ97GoVxz7NIO1WRcDoJzxedg7/g5Cncx1natd4UQ=";
-      "h263-rs-0.1.0" = "sha256-t9yb3d0tiCf8eaK3PwIxsvsw1FH3VHHLLYWbHIxyezI=";
-      "jpegxr-0.3.1" = "sha256-zmFwTn37QB/dXEC2wTmGJRmpaJFskNxja5AQMLzJvcI=";
-      "nellymoser-rs-0.1.2" = "sha256-66yt+CKaw/QFIPeNkZA2mb9ke64rKcAw/6k/pjNYY04=";
-      "nihav_codec_support-0.1.0" = "sha256-HAJS4I6yyzQzCf+vmaFp1MWXpcUgFAHPxLhfMVXmN1c=";
-    };
-  };
+  useFetchCargoVendor = true;
+  cargoHash = "sha256-q+9yhUjYolPlBt6W1xJPoyE7DgqDffEhkQqJmSX4y3Y=";
 
   meta = with lib; {
     description = "Adobe Flash Player emulator written in the Rust programming language";

Copy link
Author

@normalcea normalcea Jan 12, 2025

Choose a reason for hiding this comment

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

I thought about doing this but I didn't want to partially revert the changes made in commit f8cbc3c and I don't want to assume that git dependencies should be replaced with the new fetcher.

Though if this is permitted then I can update the PR to use cargoHash. @jchv @GovanifY thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

The new fetcher was designed for git dependencies. I don't think there is any other reason to use it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know anything about the Rust tooling in Nix, but I would be ecstatic if we could get rid of the need to include the cargo.lock file in Nixpkgs. Sounds like we can with no major downsides. So please go for it.

@GovanifY seems busy and hasn't participated a whole ton with ruffle maintainership in Nixpkgs, it might be about time to remove them from maintainers so we can stop bothering them for now.

Copy link
Member

Choose a reason for hiding this comment

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

@jchv Sure, fine by me. I was pretty busy when you started taking over the maintainership and since then you've been leading the PRs, so I've let you to it ^^". Thanks for the work since then!

Copy link
Contributor

Choose a reason for hiding this comment

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

SGTM, thanks for chiming in; one of us will probably do this at some point in a future PR. Thanks your work before me, as well! As long as I am healthy and able I'll try to continue to participate in maintainership of the Ruffle derivation indefinitely, and obviously it's a simple matter to re-add yourself anyways.

I know Ruffle has been gearing up for a "stable" release in the near future. My plan is that we can add some NixOS tests to ensure Ruffle is functioning properly and make maintainership of Ruffle low-touch, with automatic updating and testing to take care of the majority of the work.

@normalcea
Copy link
Author

I force pushed the PR to fix the issue with the desktop file on Linux which had Exec=ruffle %u instead of Exec=ruffle_desktop %u and tested its integration with the desktop using nix profile install .#ruffle

@normalcea
Copy link
Author

I again force pushed to add FliegendeWurst's recommendation to use cargoHash.

@normalcea
Copy link
Author

Another force push to rewrite meta section to remove the use of with keyword which can cause inconsistencies when used within a build attrset.[1] I also just generally dislike with when used in attrsets since the behavior isn't obvious.

@normalcea
Copy link
Author

normalcea commented Jan 13, 2025

I created a draft pr for the proposal I mentioned about creating a pre-built binary ruffle package as it solves the long-standing issue of ruffle's build determinism interfering with flash programs @jchv feel free to take a look.

#373520

@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jan 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants