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

crawl: add .desktop file and use high-res app icon #59915

Merged
merged 1 commit into from Apr 26, 2019

Conversation

Projects
None yet
3 participants
@lightbulbjim
Copy link
Contributor

commented Apr 20, 2019

Motivation for this change

The crawl source includes .desktop files but currently they are not
installed. This change installs them (with the executable path tweaked
for NixOS compatibility).

Also included in this change is a patch to use the included high-res app
icon instead of the default 32x32 icon. The default icon is very low res
and looks out of place beside other app icons.

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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@lightbulbjim

This comment has been minimized.

Copy link
Contributor Author

commented Apr 22, 2019

I've upstreamed the icon change but they only release every six months. Once the new version is released I'll remove the patch from nixpkgs, but in the meantime it would be nice to keep it.

@worldofpeace
Copy link
Member

left a comment

Can you add the commit you upstreamed as a patch like?

patches = [
  (fetchpatch {
    url = "https://github.com/crawl/crawl/commit/2aa1166087e44e6585b26cedf1fe81b3f3ba547f.patch"
    sha256 = "0000000000000000000000000000000000000000000000000000";
  })
];

And remove it from the tree?

@lightbulbjim lightbulbjim force-pushed the lightbulbjim:crawl-icon branch from a701c6f to c9a9a57 Apr 24, 2019

@lightbulbjim

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

Can you add the commit you upstreamed as a patch ... And remove it from the tree?

Good idea, it didn't even occur to me. Updated.

@worldofpeace

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

Hash mismatch

 wanted: sha256:09ip3ppb81szn85l32zh3xiyp9kblcz8d0ngw83ilyr7p796m402
  got:    sha256:1jqrdv4wy18shg1fdabdb421232hg5micphkixcyzxd1lrmvadg0

Note that fetchpatch normalizes the generated patch, so if you for example retrieved the hash from
nix-prefetch-url it won't be correct.

@lightbulbjim

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2019

Note that fetchpatch normalizes the generated patch, so if you for example retrieved the hash from
nix-prefetch-url it won't be correct.

I did use nix-prefetch-url to retrieve the hash.

The build succeeds for me with either 09ip3ppb81szn85l32zh3xiyp9kblcz8d0ngw83ilyr7p796m402 or 1jqrdv4wy18shg1fdabdb421232hg5micphkixcyzxd1lrmvadg0 set in fetchpatch. Not sure what's going on there. Other values do fail for me with a hash mismatch.

I guess I should use 1jqrdv4wy18shg1fdabdb421232hg5micphkixcyzxd1lrmvadg0? Not sure why you're getting different results (or why both hashes work for me).

crawl: add .desktop file and use high-res app icon
The crawl source includes .desktop files but currently they are not
installed. This change installs them (with the executable path tweaked
for NixOS compatibility).

Also included in this change is an upstream patch to use the included
high-res app icon instead of the default 32x32 icon. The default icon
is very low res and looks out of place beside other app icons.

@lightbulbjim lightbulbjim force-pushed the lightbulbjim:crawl-icon branch from c9a9a57 to 82d31fd Apr 26, 2019

@lightbulbjim

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2019

Finally got a chance to look at this. The tl;dr is that I updated the hash to 1jqrdv4wy18shg1fdabdb421232hg5micphkixcyzxd1lrmvadg0 and nix-review is now happy.

More context:

nix-build was happy with either hash. In fact, I've been building my system packages from a local branch with the "bad" hash for the past few days and it's been working fine. nix-build does complain if I change the hash to some other value.

nix-review, on the other other hand, would only work with 1jqrdv4wy18shg1fdabdb421232hg5micphkixcyzxd1lrmvadg0. Not sure what it's doing differently to nix-build; I'll have to look into that.

Compounding my testing was the fact that nix-review wip would fail to build the package every time with a "patch failed to apply" error. The only way I could get it to build was to commit my changes and use nix-review rev. Again, something I need to figure out as it took a while for me to realise that the error had nothing to do with the patch.

@worldofpeace

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

For fixed-output derivations if the hash already matches a path in the store nix will skip the download and just use the existing path. Since you used nix-prefetch-url and used that hash I didn't get downloaded because the path already existed.

So changing the hash so that it no longer matches forces a rebuild, and then you get the mismatch because it wasn't correct.

I tend to do Trust on first use where you use a placeholder hash that for sure will be wrong

sha256 = "0000000000000000000000000000000000000000000000000000";

and then when the build fails with a hash mismatch you then substitute it with that hash
and assume it's correct.

I've done this locally and the hash used is correct

hash mismatch in fixed-output derivation '/nix/store/7spb1nsg7dj2bji2bgpai806vy4bmja4-2aa1166087e44e6585b26cedf1fe81b3f3ba547f.patch':
  wanted: sha256:0000000000000000000000000000000000000000000000000000
  got:    sha256:1jqrdv4wy18shg1fdabdb421232hg5micphkixcyzxd1lrmvadg0
more info at https://nixos.org/nixos/nix-pills/nix-store-paths.html
@worldofpeace
Copy link
Member

left a comment

Desktop item and icons look good.

@worldofpeace worldofpeace merged commit bd89f7c into NixOS:master Apr 26, 2019

13 checks passed

Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@worldofpeace

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

Thank you @lightbulbjim for fixing this.

If you have more questions about the hash issue, feel free to ask me.
There's also lots of people in #nixos (freenode) who'd also enjoy explaining this.

@lightbulbjim

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2019

Thanks for merging and especially for the help. I appreciate it!

@lightbulbjim lightbulbjim deleted the lightbulbjim:crawl-icon branch Apr 26, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.