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-bin: init at 2025-01-13 (#285887) #373520

Closed
wants to merge 6 commits into from

Conversation

normalcea
Copy link
Contributor

@normalcea normalcea commented Jan 13, 2025

Depends on #371012 and is a proof of concept draft for adding a binary ruffle package to nixpkgs. This fixes issue #285887 as it doesn't build deterministically as well as adding support for ruffle's about page[1, prebuilt is left, native comp is right]. It also allows for automatic updating with nix-update-script --version=unstable without burdening CI.

A pre-built binary package also allows for easily pinning a version of ruffle without the burden of having to compile ruffle from source on the user's end which is very time/resource consuming as it is a complex Rust project.

I do not have access to a Darwin machine so I left the derivation for darwin as a stub. @jchv if you can fill that in that would be great. Anyone with a Linux machine can also help test (for example, on a X11 display server vs. wayland).

[1]
comparison

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.

@normalcea normalcea marked this pull request as draft January 13, 2025 18:32
@github-actions github-actions bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Jan 13, 2025
@jchv
Copy link
Contributor

jchv commented Jan 14, 2025

FWIW I did take a look at this last night, here's a first-pass of Darwin support:

diff --git a/pkgs/by-name/ru/ruffle-bin/package.nix b/pkgs/by-name/ru/ruffle-bin/package.nix
index eba8c41733ed..5d93bd5376be 100644
--- a/pkgs/by-name/ru/ruffle-bin/package.nix
+++ b/pkgs/by-name/ru/ruffle-bin/package.nix
@@ -16,6 +16,11 @@ let
   pname = "ruffle-bin";
   version = "nightly-2025-01-13";
   passthru.updateScript = nix-update-script { extraArgs = [ "--version=unstable" ]; };
+  urlPrefix =
+    version:
+    "https://github.com/ruffle-rs/ruffle/releases/download/${version}/ruffle-nightly-${
+      builtins.replaceStrings [ "-" ] [ "_" ] (lib.strings.removePrefix "nightly-" version)
+    }";
 
   x86_64-linux = stdenvNoCC.mkDerivation (finalAttrs: {
     inherit
@@ -26,9 +31,7 @@ let
       ;
 
     src = fetchurl {
-      url = "https://github.com/ruffle-rs/ruffle/releases/download/${finalAttrs.version}/ruffle-nightly-${
-        builtins.replaceStrings [ "-" ] [ "_" ] (lib.strings.removePrefix "nightly-" finalAttrs.version)
-      }-linux-x86_64.tar.gz";
+      url = "${urlPrefix finalAttrs.version}-linux-x86_64.tar.gz";
       hash = "sha256-pn+3cWgMnH06VCBgRxVGB9Dx9Kxq5IAm6ytLa744OOY=";
     };
 
@@ -71,8 +74,33 @@ let
     '';
   });
 
-  # TODO: Add Darwin support.
-  darwin = null;
+  darwin = stdenvNoCC.mkDerivation (finalAttrs: {
+    inherit
+      pname
+      version
+      passthru
+      meta
+      ;
+
+    src = fetchurl {
+      url = "${urlPrefix finalAttrs.version}-macos-universal.tar.gz";
+      hash = "sha256-q252R1IVhfa4YC/pwhxK/EEz7/3NOy2uwkMdtKIZE/Q=";
+    };
+
+    sourceRoot = ".";
+
+    installPhase = ''
+      runHook preInstall
+
+      mkdir -p "$out/Applications"
+      cp -R Ruffle.app "$out/Applications"
+
+      mkdir -p $out/bin
+      ln -s "$out/Applications/Ruffle.app/Contents/MacOS/ruffle" "$out/bin/ruffle"
+
+      runHook postInstall
+    '';
+  });
 
   meta = {
     description = "Nightly pre-built binary release of ruffle, the Adobe Flash Player emulator";

Though one issue with this is that merely linking to the binary inside the bundle for the bin binary doesn't work very well since then it will later fail to find resources in the application bundle and crash during some operations. So actually it should be a wrapper that does something akin to open $out/Applications/Ruffle.app, but I'm not sure the most idiomatic way to do this on Darwin. I'll check it out after work probably.

P.S.: I'm on-board with not using with or rec but I am not sure that using finalAttrs for version is an improvement (over just using ${version} from the let statement directly). At first blush it seems convenient but the only place it's ever used is for the fetchurl call, where you'll have to update the FOD hash any time the version changes anyways, so it doesn't make things more convenient for someone overriding.

@emilazy
Copy link
Member

emilazy commented Jan 14, 2025

It seems like the resolution in earlier issues was just that it’d take a simple build system tweak to build the part of Ruffle people actually use, or a simple patch to the build system to be able to build everything without pulling in the determinism feature in the player. I think that would be preferable to maintaining a parallel binary package.

@jchv
Copy link
Contributor

jchv commented Jan 14, 2025

Yeah, that's fair. I would prefer to fix the source derivation as well, I just haven't taken the time to dig deeper into it. I am ambivalent as to whether or not a binary derivation should or should not exist, though it seems relatively easy to do and maintain if there are any other reasons why someone might want it.

Ruffle when using openh264, will check for openh264-2.4.1.so at
runtime, if it doesn't find one, it will download it and place it in
its cache. This patch prevents that from happening.
> Though one issue with this is that merely linking to the binary
inside the bundle for the bin binary doesn't work very well since then
it will later fail to find resources in the application bundle and
crash during some operations. So actually it should be a wrapper that
does something akin to open $out/Applications/Ruffle.app, but I'm not
sure the most idiomatic way to do this on Darwin. I'll check it out
after work probably.
@github-actions github-actions bot added 10.rebuild-darwin: 1-10 and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin labels Jan 15, 2025
@normalcea
Copy link
Contributor Author

I discovered that ruffle's default behavior regarding the openh264 library is to download it if it doesn't find it at runtime. It specifically needs the previous release 2.4.1 of the library. This should probably be ported over to the compiled ruffle package as well.

It seems like the resolution in earlier issues was just that it’d take a simple build system tweak to build the part of Ruffle people actually use, or a simple patch to the build system to be able to build everything without pulling in the determinism feature in the player. I think that would be preferable to maintaining a parallel binary package.

This is right which is why I'm likely going to keep this pr as a draft for the time being till we can write up a solution for ruffle itself.

@normalcea
Copy link
Contributor Author

normalcea commented Jan 19, 2025

After hacking the package derivation recently, I discovered the easiest fix for the determinism issue is to simply create a patch file to patch out all instances of the deterministic feature from each crate's Cargo.toml file (exporter, scanner, etc). We can continue building all ruffle tools as usual.

Funnily enough there doesn't seem to be a cargo way of doing this at the moment. The reason it built deterministic was because the exporter and scanner tools had that feature enabled which built the desktop crate with the feature. Just Rust things.

diff --git a/exporter/Cargo.toml b/exporter/Cargo.toml
index 93066a30d..0f8b71ae4 100644
--- a/exporter/Cargo.toml
+++ b/exporter/Cargo.toml
@@ -13,7 +13,7 @@ workspace = true
 [dependencies]
 clap = { workspace = true }
 futures = { workspace = true }
-ruffle_core = { path = "../core", features = ["deterministic", "default_font"] }
+ruffle_core = { path = "../core", features = ["default_font"] }
 ruffle_render_wgpu = { path = "../render/wgpu", features = ["clap"] }
 image = { workspace = true, features = ["png"] }
 walkdir = { workspace = true }
diff --git a/scanner/Cargo.toml b/scanner/Cargo.toml
index 59781ba79..40cf54c24 100644
--- a/scanner/Cargo.toml
+++ b/scanner/Cargo.toml
@@ -12,7 +12,7 @@ workspace = true

 [dependencies]
 clap = { workspace = true }
-ruffle_core = { path = "../core", features = ["deterministic"] }
+ruffle_core = { path = "../core" }
 log = { workspace = true }
 walkdir = { workspace = true }
 serde = { workspace = true, features = ["derive"] }
diff --git a/tests/Cargo.toml b/tests/Cargo.toml
index 26bfc9a89..b7342b662 100644
--- a/tests/Cargo.toml
+++ b/tests/Cargo.toml
@@ -27,7 +27,7 @@ ruffle_render_wgpu = { path = "../render/wgpu", optional = true }
 regex = "1.11.1"

 [dev-dependencies]
-ruffle_core = { path = "../core", features = ["deterministic", "timeline_debug", "avm_debug", "audio", "mp3", "aac", "default_font", "test_only_as3"] }
+ruffle_core = { path = "../core", features = ["timeline_debug", "avm_debug", "audio", "mp3", "aac", "default_font", "test_only_as3"] }
 ruffle_test_framework = { path = "framework" }
 libtest-mimic = "0.8.1"
 walkdir = { workspace = true }
diff --git a/tests/framework/Cargo.toml b/tests/framework/Cargo.toml
index ffc59e25b..639028578 100644
--- a/tests/framework/Cargo.toml
+++ b/tests/framework/Cargo.toml
@@ -11,7 +11,7 @@ version.workspace = true
 workspace = true

 [dependencies]
-ruffle_core = { path = "../../core", features = ["deterministic", "timeline_debug", "avm_debug", "audio", "mp3", "aac", "default_font", "serde"] }
+ruffle_core = { path = "../../core", features = ["timeline_debug", "avm_debug", "audio", "mp3", "aac", "default_font", "serde"] }
 ruffle_render = { path = "../../render", features = ["serde"] }
 ruffle_input_format = { path = "../input-format" }
 ruffle_socket_format = { path = "../socket-format" }

Ruffle is also going to get a bump in the OpenH264 version to 2.5.0 (ruffle-rs/ruffle#18581), so when that happens I'll open a PR to revise the derivation to wrap the library like so on Linux. Otherwise, the openh264 drv has to be overridden to compile an older version which is unnecessary.

  preFixup = lib.optionalString stdenv.hostPlatform.isLinux ''
    patchelf $out/bin/ruffle_desktop \
      --add-needed libxkbcommon-x11.so \
      --add-needed libwayland-client.so \
      --add-needed libopenh264.so \
      --add-rpath ${libxkbcommon}/lib:${wayland}/lib:${openh264}/lib
  '';

After that, we basically update ruffle before each stable nixpkgs release or when it reaches stable we can add the nix-update-script { } to it and be done with it.

I will close this PR draft, it is no longer needed and I'll keep it as a reference.

@normalcea normalcea closed this Jan 19, 2025
@normalcea normalcea deleted the ruffle-bin branch January 19, 2025 04:51
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.

3 participants