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

Jump to bottom

GZDoom 4.3.3 -> 4.4.2 #90690

Merged
merged 8 commits into from Jul 16, 2020
Merged

GZDoom 4.3.3 -> 4.4.2 #90690

merged 8 commits into from Jul 16, 2020

Conversation

Derpford
Copy link
Contributor

@Derpford Derpford commented Jun 17, 2020 •
edited

4.4.0 increased the ZScript version number, GZDoom 4.4.1 and 4.4.2 have some bugfixes. I was unable to test this at all because I can't figure out how to get home-manager to install GZDoom with a specified version attribute.

Motivation for this change

Keeping up with current release version.

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.

@Derpford
Update to 4.4.2
2de8960 
4.4.0 increased the ZScript version number, GZDoom 4.4.1 and 4.4.2 have some bugfixes.
@iblech
Copy link
Contributor

iblech commented Jun 17, 2020

Hello Derpford and congratulations on your first pull request! You are welcome! :-)

I also don't know about home-manager, but you can test your update by:

nix-shell -I nixpkgs=https://github.com/Derpford/nixpkgs/archive/patch-1.tar.gz -p gzdoom

I'll do this now and report back my findings. :-)

cole-h
cole-h requested changes Jun 17, 2020
View reviewed changes
Copy link
Member

@cole-h cole-h left a comment •
edited

Choose a reason for hiding this comment

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

Hi, welcome! Thanks for contributing! There are a couple problems with your PR, however:

  • Sorry, update what to 4.4.2? ;)

    Please change your commit message to follow our contributing guidelines (located here): gzdoom: 4.3.3 -> 4.4.2.

  • Additionally, how did you get that sha256? I get a hash mismatch:

    hash mismatch in fixed-output derivation '/nix/store/b7pajg3z67jnsqj5656cclv154ninxjr-source':
  wanted: sha256:0x06069ywl3mndspyi4pq91myivpf3jkclwagbcvcd48zapfkvfh
  got:    sha256:1xkkmbsdv64wyb9r2fv5mwyqw0bjryk528jghdrh47pndmjs9a38
cannot build derivation '/nix/store/sfr086iqh5kczy7lsxrnr7fb4nyk0yg8-gzdoom-4.4.2.drv': 1 dependencies couldn't be built
cannot build derivation '/nix/store/a2szlx675q1669ppd9fgil20j0yxfli3-env.drv': 1 dependencies couldn't be built
[0 built (1 failed), 7 copied (145.8 MiB), 115.0 MiB DL]
error: build of '/nix/store/a2szlx675q1669ppd9fgil20j0yxfli3-env.drv' failed
https://github.com/NixOS/nixpkgs/pull/90690
1 package failed to build:
gzdoom

And to answer your question on how to build this yourself:

  1. cd /path/to/your/nixpkgs/checkout
  2. Make your changes
  3. nix-build -A gzdoom to verify that the gzdoom attr still builds after your change

This will create a result symlink to the built store path, where you can then run the binary from result/bin/binary.

@ofborg ofborg bot requested a review from Lassulus June 17, 2020 21:44
@Derpford Derpford changed the title Update to 4.4.2 GZDoom 4.3.3 -> 4.4.2 Jun 17, 2020
@Derpford
Copy link
Contributor Author

Re: that hash mismatch, I grabbed the hash by running nix-prefetch-url on this URL:
https://github.com/coelckers/gzdoom/archive/g4.4.2.tar.gz

@cole-h
Copy link
Member

cole-h commented Jun 17, 2020

Ah, yeah. That's because nix-prefetch-url only hashes the resulting tarball, whereas fetchFromGitHub will extract it and then hash it. To get the proper hash, you should use nix-prefetch-url --unpack https://github.com/coelckers/gzdoom/archive/g4.4.2.tar.gz (--unpack is key).

@iblech
Copy link
Contributor

iblech commented Jun 17, 2020 •
edited

I confirm that I get the same hash mismatch which cole-h reported. Fixing that, I run into an error concerning libraries/zmusic/mididevices/music_fluidsynth_mididevice.cpp. Apparently since the previously packaged version, zmusic was made into its own repository on GitHub, with gzdoom depending on it. Hence we'll require a little more changes to the Nix package. (A template for how this could look like is this package, though I cannot vouch for whether this is considered best practice, being inexperienced myself.)

As I understand it, git-prefetch-url may only be used if the Nix package actually references the tarball by URL. If instead fetchFromGitHub is used, as it is here, you need to use git-prefetch-git https://github.com/coelckers/gzdoom g4.4.2. I'm sorry that you ran into this problem. I'm also just a random passer-by, but I guess that a patch to fix the documentation to be more clear on this point will be gladly appreciated, if you'd like to contribute in this fashion!

@Derpford
Copy link
Contributor Author

I'm in over my head on this, I'm afraid. Let me link this to some friends of mine, who've been teaching me how to work with Nix.

@Derpford
Fix bad checksum
bb8f141 
Old hash was done without `--unpack` option, resulting in a mismatch.
@Derpford
Fetch submodules
ad11bf2 
This fixes the issue with `zmusic` not being brought in as a dependency.
@Derpford
Copy link
Contributor Author

Finally got back to this. Apparently zmusic isn't actually a submodule, which is annoying. I'm told I can pass it in as a buildInput instead, so I'll try that next...

@Derpford
Add zmusic derivation
ad7ff8f 
Since zmusic isn't actually a submodule, we have to derive it ourselves and pass it as a buildInput.
@Derpford
Semicolon.
7ae7af4
@Derpford
Semicolon in the *right* place this time.
0380ac6
@Derpford
Merge the zmusic source into the gzdoom source.
c3b9de6 
Apparently this is how they expect zmusic to be added. Why this isn't a submodule, I don't know.
@Derpford
Copy link
Contributor Author

Well, I've tried handling zmusic as a separate derivation, I've tried merging it into the source tree, but each time I get the same "no such file or directory" error.

@Derpford
Fix bad path editing
f97fdba 
Credit to Open Skies for fixing this. Turns out the paths edited in the `gzdoom` derivation were for zmusic, which needs these edits to be made so that it looks in nix-friendly places for its stuff.
@Derpford
Copy link
Contributor Author

Open Skies worked out a possible solution, testing it now...

@Derpford
Copy link
Contributor Author

Successfully built this version on nixos! It's working perfectly, from what I've seen.

@Lassulus Lassulus merged commit 951e217 into NixOS:master Jul 16, 2020
@Lassulus
Copy link
Member

nice! thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cole-h cole-h cole-h requested changes

@Lassulus Lassulus Awaiting requested review from Lassulus

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 10.rebuild-linux: 1-10 10.rebuild-linux: 1 12. first-time contribution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Derpford @iblech @cole-h @Lassulus @Janik-Haag