reSnap: init at 2.5.2

[404Wolf]

Description of changes

Things done

• Built on platform(s)
  • [X] x86_64-linux
  • [ ] aarch64-linux
  • [ ] x86_64-darwin
  • [ ] aarch64-darwin
• For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
  • [ ] sandbox = relaxed
  • [ ] sandbox = true
• [x] Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• [ ] Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• [x] Tested basic functionality of all binary files (usually in ./result/bin/)
• 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
  • [ ] (Package updates) Added a release notes entry if the change is major or breaking
  • [ ] (Module updates) Added a release notes entry if the change is significant
  • [ ] (Module addition) Added a release notes entry if adding a new NixOS module
• [x] Fits CONTRIBUTING.md.

---

Add a :+1: reaction to pull requests you find important.

[nixos-discourse]

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

https://discourse.nixos.org/t/prs-ready-for-review/3032/4426

[404Wolf]

Cool, I've made all the changes, but I rebased the wrong things and am fixing a git issue now. This should be ready to merge soon

[404Wolf]

I think it's all fixed and ready! Thanks again for all the great feedback!

[404Wolf]

Hey @SuperSandro2000, good call about wrapping with mkDerivation, I've made the change. If I'm reading right you want to remove explicit phases -- why is this? And how does the current state look? Thanks!

[404Wolf]

@SuperSandro2000 not to be annoying, but have you had a chance to look this over again?

[eljamm]

Hey @404Wolf, I think you're doing great so far, but I believe the package can still be improved further:

• We don't need two mkDerivation since we can put everything in one
• We don't need writeShellApplication anymore as we can just copy the script to the output
• Use finalAttrs instead of rec (see https://github.com/NixOS/nixpkgs/issues/315337).
• Whenever we override a phase like installPhase, we should always add the pre- and post- hooks.
• Contrary to my first suggestion, it's discouraged to set phases according to the stdenv docs since it's easy to forget those that run important things.

As such, here is what I think the derivation should look like:

package.nix

{
  lib,
  stdenvNoCC,
  fetchFromGitHub,
  ffmpeg,
  feh,
  imagemagick_light,
  lz4,
}:
stdenvNoCC.mkDerivation (finalAttrs: {
  pname = "resnap";
  version = "2.5.2";

  src = fetchFromGitHub {
    owner = "cloudsftp";
    repo = "reSnap";
    rev = "v${finalAttrs.version}";
    hash = "sha256-thVyf1gTDPLQVtZKoWL7SGiWI++ICWqmF/Ar57I3WP8=";
  };

  buildInputs = [
    ffmpeg
    feh
    imagemagick_light
    lz4
  ];

  installPhase = ''
    runHook preInstall

    install -D ./reSnap.sh $out/bin/reSnap

    runHook postInstall
  '';

  postFixup = ''
    substituteInPlace $out/bin/reSnap \
      --replace-fail "\$0" "reSnap"
  '';

  meta = {
    description = "Take screnshots of your reMarkable tablet over SSH";
    homepage = "https://github.com/cloudsftp/reSnap";
    license = with lib.licenses; [ mit ];
    maintainers = with lib.maintainers; [ _404wolf ];
    mainProgram = "reSnap";
  };
})

Let me know what you think and if you have any questions regarding this, don't hesitate to ask.

[404Wolf]

Hey @404Wolf, I think you're doing great so far, but I believe the package can still be improved further:

* We don't need two `mkDerivation` since we can put everything in one

* We don't need `writeShellApplication` anymore as we can just copy the script to the output

* Use `finalAttrs` instead of `rec` (see [Documentation: guide for using `let in` vs `rec` vs `finalAttrs` #315337](https://github.com/NixOS/nixpkgs/issues/315337)).

* Whenever we override a phase like `installPhase`, we should always add the `pre-` and `post-` hooks.

* Contrary to my first suggestion, it's discouraged to set `phases` according to the [stdenv docs](https://github.com/NixOS/nixpkgs/blob/420d0cc6f27b2b7f117be67aa721dd7154ffb003/doc/stdenv/stdenv.chapter.md#phases-var-stdenv-phases) since it's easy to forget those that run important things.

As such, here is what I think the derivation should look like: package.nix

Let me know what you think and if you have any questions regarding this, don't hesitate to ask.

I could be wrong, but I don't think that buildInputs are available at runtime; it didn't seem to work. I cleaned it up a bit by only using one mkDerivation though, and just manually append to the PATH instead of writeShellApplication, and by reusing the name. Let me know what you think. Thanks for looking it over :)

[eljamm]

I believe runtimeInputs will work in this case, but if that doesn't, we can wrap the executable with the PATH:

  nativeBuildInputs = [ makeWrapper ];

  runtimeInputs = [
    ffmpeg
    feh
    imagemagick_light
    lz4
  ];

  postFixup = ''
    substituteInPlace $out/bin/reSnap \
      --replace-fail "\$0" "reSnap"

    wrapProgram $out/bin/reSnap \
      --set PATH "${lib.makeBinPath finalAttrs.runtimeInputs}"
  '';

[404Wolf]

Hey @SuperSandro2000, any chance you could look this over at some point? I think it's ready to merge. Thanks!

[nixos-discourse]

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

https://discourse.nixos.org/t/prs-ready-for-review/3032/4607

[wolfgangwalther]

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 334700

---

x86_64-linux

:white_check_mark: 1 package built:

• resnap

---

x86_64-darwin

:white_check_mark: 1 package built:

• resnap

---

aarch64-darwin

:x: 1 package failed to build:

• resnap

---

The build failure on aarch64-darwin is because I wasn't able to build imagemagick, not sure why. Unrelated to this PR.

[404Wolf]

@wolfgangwalther thanks! Sorry for not following up myself, I'm excited to see this get merged.