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

mindustry: init at 99 (and updates) #72306

Merged
merged 5 commits into from Jan 13, 2020
Merged

Conversation

fgaz
Copy link
Member

@fgaz fgaz commented Oct 30, 2019

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nix-review --run "nix-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.
Notify maintainers

cc @

@wamserma
Copy link
Member

wamserma commented Nov 3, 2019

LGTM. Tested on NixOS 19.09.

@ghost
Copy link

ghost commented Nov 3, 2019

LGTM

@fgaz fgaz force-pushed the mindustry/init branch 2 times, most recently from 7f195dc to 5e834f1 Compare November 9, 2019 10:53
@fgaz
Copy link
Member Author

fgaz commented Nov 25, 2019

v100 is out, I just pushed an update. Is there anything blocking this?

@fgaz
Copy link
Member Author

fgaz commented Dec 13, 2019

Updated to 101

@fgaz
Copy link
Member Author

fgaz commented Dec 22, 2019

Updated to 101.1

...yes, I'll continue to ping everyone here at every release until this gets merged or rejected ;-)

@jappeace
Copy link
Contributor

please merge this

@fgaz
Copy link
Member Author

fgaz commented Jan 1, 2020

Updated to 102 and rebased

@fgaz fgaz changed the title mindustry: init at 99 mindustry: init at 99 (and updates) Jan 1, 2020
@greizgh
Copy link
Contributor

greizgh commented Jan 5, 2020

Built and tested successfully on x86_64

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

I think this package has above-average quality and can be merged. I have built it using sandboxing on NixOS x86_64 and tested the basic functionality.

  • One small wish/suggestion would be to package the server as a seperate derivation, to prevent pulling in pulseaudio on a headless machine, but I can also propose the change after it was merged.

  • Also I noticed it does not seem to like SDL_VIDEODRIVER=wayland, which I have set in my environment, but this is a problem for upstream. It might be good to add set SDL_VIDEODRIVER to x11 explicitly in the wrapper for users who have set it to wayland in their environment (because most applications just work with it).

    [I] Initialized Discord rich presence.
    arc.backend.sdl.SDLError: wayland not available
    	at arc.backend.sdl.SdlApplication.check(SdlApplication.java:181)
    	at arc.backend.sdl.SdlApplication.init(SdlApplication.java:80)
    	at arc.backend.sdl.SdlApplication.<init>(SdlApplication.java:30)
    	at mindustry.desktop.DesktopLauncher.main(DesktopLauncher.java:46)
    [I] Sending crash report.
    [I] Crash sent successfully.
    

    This is not critical though because every person who has set this variable manually in their setup will know what to do from the error message.

  • You may want to squash all of those commits together.

@peterhoeg peterhoeg merged commit 36f084b into NixOS:master Jan 13, 2020
dtzWill pushed a commit to dtzWill/nixpkgs that referenced this pull request Jan 13, 2020
* mindustry: init at 99

* mindustry: 99 -> 100

* mindustry: 100 -> 101

* mindustry: 101 -> 101.1

* mindustry: 101.1 -> 102

(cherry picked from commit 36f084b)
@fgaz
Copy link
Member Author

fgaz commented Jan 17, 2020

@petabyteboy
I reported the wayland issue upstream: Anuken/Mindustry#1393
Splitting it is a good idea. Opened #77893 to track it.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/january-2020-in-nixos/5771/1

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

8 participants