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

lunar-client: init at version 2.4.0 #103229

Merged
merged 2 commits into from Nov 16, 2020
Merged

Conversation

zyansheep
Copy link
Contributor

@zyansheep zyansheep commented Nov 9, 2020

Motivation for this change

Lunar Client is a pretty popular minecraft client / launcher

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
    ^^^ This is on by default on NixOS right?
  • 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.

Copy link
Contributor

@OPNA2608 OPNA2608 left a comment

Choose a reason for hiding this comment

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

Please fix the commit naming and history. The last 3 fix-ups should be squashed into the package init commit and the entry in the maintainer-list is usually a stand-alone commit before your first packaging commit.

git reset --soft HEAD~5 # throws away last 5 commits' information without resetting file changes
git reset HEAD pkgs/games/lunar-client/default.nix # remove lunar-client from staging area
git commit # "maintainers: add zyansheep"
git add pkgs/games/lunar-client/default.nix
git commit # "lunar-client: init at 2.4.0"

git log
# when you think the git history looks good
git push -f

Copy link
Contributor

@OPNA2608 OPNA2608 left a comment

Choose a reason for hiding this comment

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

I'll test the build & binary after work today, will report back.

pkgs/top-level/all-packages.nix Show resolved Hide resolved
Copy link
Contributor

@OPNA2608 OPNA2608 left a comment

Choose a reason for hiding this comment

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

LGTM.

Result of nixpkgs-review pr 103229 1

1 package built:
  • lunar-client

@zyansheep
Copy link
Contributor Author

What else needs to be done to get this merged?

@OPNA2608
Copy link
Contributor

Someone with merge permissions needs to see this PR and merge it, that's it really. Takes abit of patience and persistence to find someone sometimes, you can try posting a link to your reviewed PR here.

@zyansheep
Copy link
Contributor Author

zyansheep commented Nov 16, 2020

Hmm... Seems like there needs to be a better way to add packages then to rely on a (relatively) small group of people to vet each and every one. Perhaps something like the Arch User Repository is needed? Maybe I'll open an rfc...

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/prs-already-reviewed/2617/285

@OPNA2608
Copy link
Contributor

Perhaps something like the Arch User Repository is needed? Maybe I'll open an rfc...

You're looking for the NUR then.

@AndersonTorres
Copy link
Member

Looks unfree to me, looks good to Borg :)

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

6 participants