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

imv: 4.1.0 -> 4.2.0 #108309

Merged
merged 3 commits into from Jan 7, 2021
Merged

imv: 4.1.0 -> 4.2.0 #108309

merged 3 commits into from Jan 7, 2021

Conversation

sternenseemann
Copy link
Member

@sternenseemann sternenseemann commented Jan 3, 2021

  • switch build system to meson
  • enable all backends by default

Open questions:

  • imv prints a warning from the tiff backend everytime a non tiff file
    is opened: Is this normal? Seems harmless enough though.
  • Should we make backends configurable / optional? I readded some
    backends which apparently were removed, but still given as an argument
    to the derivation.

Resolves #108185.

cc @markus1189 @rnhmjoj

Motivation for this change
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/) (only wayland)
  • 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.

* switch build system to wayland
* enable all backends by adding the following to buildInputs as meson
  autodetects which backends are available.
  * libtiff
  * libheif
  * libpng

Open questions:

* imv prints a warning from the tiff backend everytime a non tiff file
  is opened: Is this normal? Seems harmless enough though.
* Should we make backends configurable / optional? I readded some
  backends which apparently were removed, but still given as an argument
  to the derivation.

Resolves NixOS#108185.
Copy link
Member

@mdlayher mdlayher left a comment

Choose a reason for hiding this comment

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

LGTM.

$ nixpkgs-review pr 108309
$ git -c fetch.prune=false fetch --force https://github.com/NixOS/nixpkgs master:refs/nixpkgs-review/0 pull/108309/head:refs/nixpkgs-review/1
remote: Enumerating objects: 10, done.
remote: Counting objects: 100% (10/10), done.
remote: Compressing objects: 100% (5/5), done.
remote: Total 12 (delta 5), reused 5 (delta 5), pack-reused 2
Unpacking objects: 100% (12/12), 13.63 KiB | 4.54 MiB/s, done.
From ssh://github.com/NixOS/nixpkgs
   4d0e996b71d..712830d101d  master                -> refs/nixpkgs-review/0
 + 9d0164d53d4...a1e9a94785c refs/pull/108309/head -> refs/nixpkgs-review/1  (forced update)
$ git worktree add /home/matt/.cache/nixpkgs-review/pr-108309/nixpkgs 712830d101d94ae29a2310d91b4d54c0132f5716
Preparing worktree (detached HEAD 712830d101d)
HEAD is now at 712830d101d Merge pull request #108310 from mdlayher/mdl-bump-corerad
$ git merge --no-commit a1e9a94785cf885487d823790aa82dc82fe5924c
Automatic merge went well; stopped before committing as requested
$ nix build --no-link --keep-going --option build-use-sandbox relaxed -f /home/matt/.cache/nixpkgs-review/pr-108309/build.nix
[3 built, 21 copied (127.9 MiB), 22.4 MiB DL]
https://github.com/NixOS/nixpkgs/pull/108309
1 package built:
imv

$ nix-shell /home/matt/.cache/nixpkgs-review/pr-108309/shell.nix

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Jan 3, 2021

You beat me to it: I have just been working on this: rnhmjoj@f288f7ac25e.

imv prints a warning from the tiff backend everytime a non tiff file
is opened: Is this normal? Seems harmless enough though.

It doesn't in 4.1.0, it's probably a bug but imv seems to work just fine.

Should we make backends configurable / optional? I readded some

I would, but It doesn't look like they can be configured at all: the readme says most are optional but meson seems to require them all: maybe it's a bug in the build system. In any case, I would make X11/wayland configurable: you can pick from my commit if you want.

@sternenseemann
Copy link
Member Author

It doesn't in 4.1.0, it's probably a bug but imv seems to work just fine.

imv 4.1.0 is compiled without libtiff in nixpkgs.

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Jan 4, 2021

There's an upstream issue for it: eXeC64/imv#252

@SuperSandro2000
Copy link
Member

Result of nixpkgs-review pr 108309 run on x86_64-darwin 1

1 package marked as broken and skipped:
  • imv

@SuperSandro2000
Copy link
Member

This is a semi-automatic executed nixpkgs-review which does not build all packages (e.g. lumo, tensorflow or pytorch)
If you find some bugs or got suggestions for further things to search or run please reach out to SuperSandro2000 on IRC.

Result of nixpkgs-review pr 108309 run on x86_64-linux 1

1 package built:
  • imv

@sternenseemann
Copy link
Member Author

@rnhmjoj made imv (very) configurable via overrides in e635fca398bafbfe4f4f7cf4771d474c8e5b29b9

pkgs/applications/graphics/imv/default.nix Outdated Show resolved Hide resolved
pkgs/applications/graphics/imv/default.nix Outdated Show resolved Hide resolved
pkgs/applications/graphics/imv/default.nix Show resolved Hide resolved
@sternenseemann sternenseemann force-pushed the imv-4.2.0 branch 2 times, most recently from 5d57a6b to 3408d19 Compare January 4, 2021 15:48
@ofborg ofborg bot requested a review from rnhmjoj January 4, 2021 15:57
@SuperSandro2000
Copy link
Member

SuperSandro2000 commented Jan 4, 2021

Why once of the sudden do we need to disable backends? Can we just build them all and be done? The package is really overengineered right now.

Options are great if you want to disable options by default or there is a reason to disable them but here I can see none.

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Jan 4, 2021

Why once of the sudden do we need to disable backends? Can we just build them all and be done? The package is really overengineered right now.

Closure size, for instance: disabling GIF and SVG, which are rarely used, saves about 20MB, TIFF saves 40MB, and more for others.

imv is a very minimal image viewer and was designed to have optional backends, so it's should definitely be a feature of the package.

pkgs/applications/graphics/imv/default.nix Outdated Show resolved Hide resolved
pkgs/applications/graphics/imv/default.nix Outdated Show resolved Hide resolved
The following new derivation inputs are added:
* withBackends: a list of all backends to enable. The valid names are
  the same as specified in imv's meson_options.txt. Default is to enable
  all 7 backends: freeimage, libtiff, libjpeg(_turbo), libpng, librsvg,
  libnsgif and libheif
* withWindowSystem: either all, x11 or wayland to describe window system
  support.
Copy link
Contributor

@rnhmjoj rnhmjoj left a comment

Choose a reason for hiding this comment

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

Looks all good. Thank you!

@rnhmjoj rnhmjoj merged commit 29aefd4 into NixOS:master Jan 7, 2021
@sternenseemann sternenseemann deleted the imv-4.2.0 branch January 7, 2021 19:21
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.

imv: release 4.2.0 is out
4 participants