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

gargoyle: 2018.10.06 -> 2019.01.01 #85576

Merged
merged 1 commit into from Jun 21, 2020
Merged

gargoyle: 2018.10.06 -> 2019.01.01 #85576

merged 1 commit into from Jun 21, 2020

Conversation

emacsomancer
Copy link
Contributor

  • latest released version
Motivation for this change

update to latest release

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.
Notes
  • there are more commits since the last release, including improving some things for macOS, so building from one of the later post-release commits could have advantages. I don't have a mac to test on one way or other though.

@drewrisinger
Copy link
Contributor

Build fails locally due to incorrect hash for GitHub source.

@GrahamcOfBorg build gargoyle

@emacsomancer
Copy link
Contributor Author

Build fails locally due to incorrect hash for GitHub source.

@GrahamcOfBorg build gargoyle

I think I must not be deriving it properly then. What's the best method of doing this?

@drewrisinger
Copy link
Contributor

I think the proper way is something like (from command-line)

nix-prefetch-git

I usually just go the lazy way and copy the SHA256 that Nix puts on the command line though.

@drewrisinger
Copy link
Contributor

You should always try to build it locally via nix-build -A gargoyle before pushing a PR. saves everyone time. And if you can't for some reason, then mark your PR as draft and ask OfBorg (i.e. GrahamcOfBorg like I did above) to build your package, though it's nice to limit your builds due to OfBorg being a shared resource.

@emacsomancer
Copy link
Contributor Author

You should always try to build it locally via nix-build -A gargoyle before pushing a PR. saves everyone time. And if you can't for some reason, then mark your PR as draft and ask OfBorg (i.e. GrahamcOfBorg like I did above) to build your package, though it's nice to limit your builds due to OfBorg being a shared resource.

For some reason I must have altered the sha256 after building, but before pushing, because I've been running 2019.01.01 on my system locally for a while now.

@drewrisinger
Copy link
Contributor

drewrisinger commented Apr 29, 2020 via email

@emacsomancer
Copy link
Contributor Author

Strange. I can try to review sometime if you update the hash.

I already did (before replying), and force pushed the branch.

@drewrisinger
Copy link
Contributor

drewrisinger commented Apr 29, 2020 via email

drewrisinger
drewrisinger approved these changes Apr 29, 2020
View reviewed changes
Copy link
Contributor

@drewrisinger drewrisinger left a comment

Choose a reason for hiding this comment

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

Passes nixpkgs-review pr 85576

Not tested locally.

https://github.com/NixOS/nixpkgs/pull/85576
1 package built:
gargoyle

danieldk
danieldk suggested changes Jun 18, 2020
View reviewed changes
pkgs/games/gargoyle/default.nix Outdated Show resolved Hide resolved
pkgs/games/gargoyle/default.nix Outdated
@@ -20,13 +20,13 @@ let
in

stdenv.mkDerivation {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
stdenv.mkDerivation {
stdenv.mkDerivation rec {

pkgs/games/gargoyle/default.nix Outdated Show resolved Hide resolved
danieldk
danieldk suggested changes Jun 20, 2020
View reviewed changes
Copy link
Contributor

@danieldk danieldk left a comment

Choose a reason for hiding this comment

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

Thanks for the changes!

Looks almost good to go. It looks like you accidentally used name in place of pname. Also one small comment about the ordering in src.

pkgs/games/gargoyle/default.nix Outdated Show resolved Hide resolved
pkgs/games/gargoyle/default.nix
sha256 = "0icwgc25gp7krq6zf66hljydc6vps6bb4knywnrfgnfcmcalqqx9";
rev = version;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you move the rev attribute between the repo and sha256 attributes? Doesn't change the evaluation of the derivation, but this is the more canonical ordering in nixpkgs.

@danieldk
Copy link
Contributor

Somewhere along the way, you switched the hash (sha256) to that of the previous version. I am getting

0w54avmbp4i4zps2rb4acmpa641s6wvwbrln4vbdhcz97fx48nzz

as the hash.

@emacsomancer
gargoyle: 2018.10.06 -> 2019.01.01
37d231a 
- latest released version

- changes: adopt suggestions + more fixes
danieldk
danieldk approved these changes Jun 21, 2020
View reviewed changes
Copy link
Contributor

@danieldk danieldk left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Result of nixpkgs-review pr 85576 1

1 package built:
- gargoyle

Tested running gargoyle.

@danieldk danieldk merged commit 9993d6c into NixOS:master Jun 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danieldk danieldk danieldk approved these changes

@drewrisinger drewrisinger drewrisinger approved these changes

Assignees
No one assigned
Labels
10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@emacsomancer @drewrisinger @danieldk