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

zeroad: 0.0.23b -> 0.0.24b #114031

Merged
merged 2 commits into from
Mar 21, 2021
Merged

zeroad: 0.0.23b -> 0.0.24b #114031

merged 2 commits into from
Mar 21, 2021

Conversation

nagy
Copy link
Member

@nagy nagy commented Feb 22, 2021

Motivation for this change

This unbreaks and updates the game but it depends on #114030 .
More on why that patch to spidermonkey is needed can be read here:
https://wildfiregames.com/forum/topic/33893-spidermonkey-68-78-upgrade/

Once that PR has been merged, I will unmark the draft status of this. Any automatic build will likely fail but that should resolve once that has been merged.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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.

@r-rmcgibbo
Copy link

Result of nixpkgs-review pr 114031 at abeb2bc5 run on x86_64-linux 1

1 package failed to build:
  • zeroad

pkgs/games/0ad/data.nix Outdated Show resolved Hide resolved
Copy link
Member

@chvp chvp left a comment

Choose a reason for hiding this comment

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

I had to add a sourceRoot = "."; to 0ad/data.nix to make this work. Also, did you check whether this version of 0ad builds with gcc10? The stdenv was set to gcc9 manually when the default changed because 0.0.23 didn't with build with 10, but I would assume that that isn't the case for 0.0.24.

@nagy
Copy link
Member Author

nagy commented Feb 24, 2021

I had to add a sourceRoot = "."; to 0ad/data.nix to make this work.

What do you mean? This is added in line 12 of the file.

Also, did you check whether this version of 0ad builds with gcc10? The stdenv was set to gcc9 manually when the default changed because 0.0.23 didn't with build with 10, but I would assume that that isn't the case for 0.0.24.

I did not. But once the spidermonkey PR is merged I have to review this again, then i will test gcc10.

@chvp
Copy link
Member

chvp commented Feb 24, 2021

What do you mean? This is added in line 12 of the file.

Oh, I'm blind apparently. Sorry about the noise.

@mfsch
Copy link
Contributor

mfsch commented Feb 25, 2021

There’s also a slightly newer release "Alpha 24b" on the download page. I couldn’t find any info on what has been changed with respect to the first "Alpha 24" release, but it’s probably a good idea to update to the latest version.

Also, when I wrote my own derivation for this new Alpha, I had to change wxGTK to wxGTK31 since I got an error that only wxWidgets >= 3.0 was supported, but apparently it is working with v2.8 for you?

Finally, is it a good idea to remove the patch for the minor version check? From what I understand, that would mean that any update of spidermonkey_78 to a version != 78.6 would break the 0 A.D. build.

@nagy
Copy link
Member Author

nagy commented Feb 25, 2021

Also, when I wrote my own derivation for this new Alpha, I had to change wxGTK to wxGTK31 since I got an error that only wxWidgets >= 3.0 was supported, but apparently it is working with v2.8 for you?

I was able to get the game started. I did not test much further than that. Do you get a compilation error ?

Finally, is it a good idea to remove the patch for the minor version check? From what I understand, that would mean that any update of spidermonkey_78 to a version != 78.6 would break the 0 A.D. build.

I have something prepared for that.

@chvp
Copy link
Member

chvp commented Feb 25, 2021

I played two games last night using this pull request (and #114030), so I did not have problems with wxGTK.

@nagy
Copy link
Member Author

nagy commented Feb 25, 2021

There’s also a slightly newer release "Alpha 24b" on the download page. I couldn’t find any info on what has been changed with respect to the first "Alpha 24" release, but it’s probably a good idea to update to the latest version.

This here is the change between the two versions: 0ad/0ad@A24...A24b
Looks mostly like language updates but it should not hurt to update to that.

@zeri42 zeri42 mentioned this pull request Feb 25, 2021
10 tasks
pkgs/games/0ad/data.nix Outdated Show resolved Hide resolved
@nagy nagy changed the title zeroad: 0.0.23b -> 0.0.24 zeroad: 0.0.23b -> 0.0.24b Feb 28, 2021
@nagy
Copy link
Member Author

nagy commented Feb 28, 2021

The spidermonkey PR has since been merged (into staging). However it is not the exact same version that 0ad requires. Therefore I had to put in a little workaround to that. We are now building a new derivation of spidermonkey with a patched src attribute. Without this, we get compilation errors.

0ad also ships with this version of spidermonkey, however I would prefer the current way of doing it over building the shipped version of spidermonkey, because this way we do not need to redeclare all of spidermonkeys dependencies in this derivation.

Because spidermonkey has still not arrived in master, any automatic build will fail again. I am however removing the draft status of this, since the problem should resolve once it hits master.

@nagy nagy marked this pull request as ready for review February 28, 2021 18:21
pkgs/games/0ad/game.nix Show resolved Hide resolved
let
# the game requires a special version 78.6.0 of spidermonkey, otherwise
# we get compilation errors. We override the src attribute of spidermonkey_78
# in order to reuse that declartion, while giving it a different source input.
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this is the correct solution. The second patch in the derivation for 0.0.23b removed the minor version check, so maybe that's the correct solution here as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, i tried to remove that check already and it gives a compilation error.

Copy link
Contributor

@mfsch mfsch Mar 1, 2021

Choose a reason for hiding this comment

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

The Fedora patch contains two changes – if you remove the second change, which seems no longer necessary, and perhaps fix the line numbers to @@ -81,19 +81,6 @@ it does apply. But I tend to agree that your solution of overriding the spidermonkey_78 version is better, since 78.6 is the version that is tested by the 0 A.D. devs.

@mfsch
Copy link
Contributor

mfsch commented Mar 1, 2021

Also, when I wrote my own derivation for this new Alpha, I had to change wxGTK to wxGTK31 since I got an error that only wxWidgets >= 3.0 was supported, but apparently it is working with v2.8 for you?

I was able to get the game started. I did not test much further than that. Do you get a compilation error ?

I just saw that this is done in all-packages.nix.

@chvp
Copy link
Member

chvp commented Mar 19, 2021

The spidermonkey change necessary for this PR was merged to master a few hours ago. I'm getting a build failure for spidermonkey 78.6.0:

make[3]: Entering directory '/build/firefox-78.6.0/obj/js/src/rust'
js/src/rust/force-cargo-library-build
/nix/store/0hkwhcpjf1p32p85bc555bpykvh220kz-cargo-1.50.0/bin/cargo rustc  --release --manifest-path /build/firefox-78.6.0/js/src/rust/Cargo.toml -vv --lib --target=x86_64-unknown-linux-gnu  --  -Clto
error: failed to parse manifest at `/build/firefox-78.6.0/Cargo.toml`

Caused by:
  dependency (nix) specification is ambiguous. Only one of `branch`, `tag` or `rev` is allowed.
make[3]: *** [/build/firefox-78.6.0/config/makefiles/rust.mk:299: force-cargo-library-build] Error 101
make[3]: Leaving directory '/build/firefox-78.6.0/obj/js/src/rust'
make[2]: *** [/build/firefox-78.6.0/config/recurse.mk:74: js/src/rust/target] Error 2
make[2]: Leaving directory '/build/firefox-78.6.0/obj'
make[1]: *** [/build/firefox-78.6.0/config/recurse.mk:34: compile] Error 2
make[1]: Leaving directory '/build/firefox-78.6.0/obj'
make: *** [/build/firefox-78.6.0/config/rules.mk:390: default] Error 2

@chvp
Copy link
Member

chvp commented Mar 20, 2021

I was able to fix the build error by adding the following patch to the spidermonkey_78_6 derivation: https://github.com/chvp/nixpkgs/blob/a1a03554af1186669a645609e25a9476f267280b/pkgs/games/0ad/spidermonkey-cargo-toml.patch

@nagy
Copy link
Member Author

nagy commented Mar 20, 2021

@chvp thanks for the patch. I can happily apply it, but i would also like to understand why this is only needed for the 78.6.0 version and not the 78.8.0 one. Do you happen to know why ?

@chvp
Copy link
Member

chvp commented Mar 20, 2021

The branch, tag or rev uniqueness requirement was added in cargo 1.50 (which was also part of this staging cycle, AFAICT): https://github.com/rust-lang/cargo/blob/master/CHANGELOG.md#cargo-150-2021-02-11
The tarball for 78.8.0 contains a compliant Cargo.toml, the tarball for 78.6.0 doesn't.

@nagy
Copy link
Member Author

nagy commented Mar 20, 2021

Updated. It builds for me locally as well.

Copy link
Member

@chvp chvp left a comment

Choose a reason for hiding this comment

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

Started a game using this exact patch, and I've played at least 10 games using some version of this patch over the past few weeks.

@SuperSandro2000 SuperSandro2000 merged commit 9e99491 into NixOS:master Mar 21, 2021
};

nativeBuildInputs = [ python2 perl pkg-config ];

buildInputs = [
spidermonkey_38 boost icu libxml2 libpng libjpeg
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can now drop spidermonkey_38.

Copy link
Member Author

Choose a reason for hiding this comment

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

@nagy nagy deleted the zeroad branch March 21, 2021 10:14
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.

5 participants