reSnap: init at 2.5.2 #334700

404Wolf · 2024-08-14T19:30:06Z

Description of changes

Things done

Add a 👍 reaction to pull requests you find important.

nixos-discourse · 2024-08-19T16:53:52Z

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

eljamm · 2024-08-19T19:16:15Z

pkgs/by-name/re/resnap/package.nix

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


Suggested change

meta = with lib; {

description = " Take screnshots of your reMarkable tablet over SSH";

homepage = "https://github.com/cloudsftp/reSnap";

license = with licenses; [mit];

maintainers = with maintainers; [_404wolf];

mainProgram = "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";

};

eljamm · 2024-08-19T19:20:19Z

pkgs/by-name/re/resnap/package.nix

+  runtimeInputs = with pkgs; [
+    ffmpeg
+    feh
+    imagemagick_light
+    lz4
+  ];


Don't use pkgs inside the derivation. Instead, add the dependencies to the inputs.

Suggested change

runtimeInputs = with pkgs; [

ffmpeg

feh

imagemagick_light

lz4

];

runtimeInputs = [

ffmpeg

feh

imagemagick_light

lz4

];

eljamm · 2024-08-19T19:21:10Z

pkgs/by-name/re/resnap/package.nix

+  lib,
+  pkgs,
+  fetchFromGitHub,
+  writeShellApplication,


Don't use pkgs inside the derivation. Instead, add the dependencies to the inputs.

Suggested change

lib,

pkgs,

fetchFromGitHub,

writeShellApplication,

lib,

fetchFromGitHub,

writeShellApplication,

ffmpeg,

feh,

imagemagick_light,

lz4,

eljamm · 2024-08-19T19:22:28Z

pkgs/by-name/re/resnap/package.nix

+    lz4
+  ];
+
+  text = "${fetchFromGitHub {


File should be formatted with nixfmt

eljamm · 2024-08-19T20:45:06Z

pkgs/by-name/re/resnap/package.nix

+  fetchFromGitHub,
+  writeShellApplication,
+}:
+writeShellApplication {


Since we're using writeShellApplication, we don't have a version or src attributes, but they're pretty useful to have, so we can do something like this:

Suggested change

writeShellApplication {

let

name = "reSnap";

version = "2.5.2";

src = fetchFromGitHub {

owner = "cloudsftp";

repo = "reSnap";

rev = "v${version}";

hash = "sha256-P+q5NMGtXE2XCcGGHZJGw8RoOqJSHSv7oyMLkn/ltfY=";

};

in

writeShellApplication {

inherit name;

runtimeInputs = [

feh

ffmpeg

imagemagick_light

lz4

];

text = "${src}/reSnap.sh";

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";

};

}

Also, the latest git revision seems to be the same as the latest tagged release (2.5.2), so we should use that, instead.

Can you update the commit message and the PR title to reflect this?

404Wolf · 2024-08-20T06:54:27Z

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 · 2024-08-20T07:02:04Z

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

eljamm · 2024-08-20T07:43:30Z

pkgs/by-name/re/resnap/package.nix

+  name = "reSnap";
+  version = "2.5.2";
+
+  src = stdenv.mkDerivation {


I see that you're using mkDerivation to patch the script name. However, we should use stdenvNoCC since we're not compiling anything.

Suggested change

src = stdenv.mkDerivation {

src = stdenvNoCC.mkDerivation {

Oh, super cool, thanks for showing me this!

pkgs/by-name/re/resnap/package.nix

eljamm

LGTM!

SuperSandro2000 · 2024-08-22T09:56:41Z

maintainers/maintainer-list.nix

Please rebase

pkgs/by-name/re/resnap/package.nix

SuperSandro2000 · 2024-08-22T09:57:11Z

pkgs/by-name/re/resnap/package.nix

+    phases = [
+      "unpackPhase"
+      "patchPhase"
+      "installPhase"
+    ];


Suggested change

phases = [

"unpackPhase"

"patchPhase"

"installPhase"

];

SuperSandro2000 · 2024-08-22T09:57:46Z

pkgs/by-name/re/resnap/package.nix

+    '';
+  };
+in
+writeShellApplication {


Why not copy the code from src to a subdirectory in out and have a proper application with pname, version, etc?

SuperSandro2000 · 2024-08-22T09:57:52Z

pkgs/by-name/re/resnap/package.nix

+      "installPhase"
+    ];
+    postPatch = ''
+      substituteInPlace ./reSnap.sh --replace-quiet "\$0" "reSnap"


Suggested change

substituteInPlace ./reSnap.sh --replace-quiet "\$0" "reSnap"

substituteInPlace ./reSnap.sh --replace-fail "\$0" "reSnap"

pkgs/by-name/re/resnap/package.nix

404Wolf · 2024-08-22T15:31:33Z

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 · 2024-08-26T22:41:08Z

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

eljamm · 2024-08-27T00:21:20Z

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).
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 · 2024-08-27T16:16:24Z

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 · 2024-08-27T21:14:52Z

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}"
  '';

eljamm · 2024-08-27T21:19:47Z

pkgs/by-name/re/resnap/package.nix

+    homepage = "https://github.com/cloudsftp/reSnap";
+    license = with lib.licenses; [ mit ];
+    maintainers = with lib.maintainers; [ _404wolf ];
+    mainProgram = name;


Changes to meta should not cause a rebuild, which would happen if name changes. We should explicitly set this.

Suggested change

mainProgram = name;

mainProgram = "reSnap";

eljamm · 2024-08-27T21:21:01Z

pkgs/by-name/re/resnap/package.nix

+  installPhase = ''
+    runHook preInstall
+
+    mkdir -p $out/bin
+    echo PATH="\$PATH:${
+      lib.strings.makeBinPath [
+        ffmpeg
+        feh
+        imagemagick_light
+        lz4
+      ]
+    }" > $out/bin/reSnap
+    cat ./reSnap.sh >> $out/bin/reSnap
+    chmod +x $out/bin/reSnap
+
+    runHook postInstall
+  '';


We can use runtimeInputs or wrap the executable instead of doing this.

Suggested change

installPhase = ''

runHook preInstall

mkdir -p $out/bin

echo PATH="\$PATH:${

lib.strings.makeBinPath [

ffmpeg

feh

imagemagick_light

lz4

]

}" > $out/bin/reSnap

cat ./reSnap.sh >> $out/bin/reSnap

chmod +x $out/bin/reSnap

runHook postInstall

'';

installPhase = ''

runHook preInstall

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

runHook postInstall

'';

SuperSandro2000 · 2024-08-28T09:45:04Z

pkgs/by-name/re/resnap/package.nix

+
+  postFixup = ''
+    substituteInPlace $out/bin/reSnap \
+      --replace-fail "\$0" ${name}


Suggested change

--replace-fail "\$0" ${name}

--replace-fail "\$0" reSnap

SuperSandro2000 · 2024-08-28T09:45:16Z

pkgs/by-name/re/resnap/package.nix

+  pname = lib.strings.toLower name;
+  version = "2.5.2";
+
+  src = fetchFromGitHub {
+    owner = "cloudsftp";
+    repo = name;


Suggested change

pname = lib.strings.toLower name;

version = "2.5.2";

src = fetchFromGitHub {

owner = "cloudsftp";

repo = name;

pname = "resnap;

version = "2.5.2";

src = fetchFromGitHub {

owner = "cloudsftp";

repo = "reSnap";

SuperSandro2000 · 2024-08-28T09:45:23Z

pkgs/by-name/re/resnap/package.nix

+let
+  name = "reSnap";
+in


Suggested change

let

name = "reSnap";

in

eljamm

LGTM

404Wolf · 2024-09-13T03:48:28Z

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

nixos-discourse · 2024-09-24T05:02:04Z

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

lelgenio · 2024-09-30T14:54:35Z

pkgs/by-name/re/resnap/package.nix

+  postFixup = ''
+    substituteInPlace $out/bin/reSnap \
+      --replace-fail "\$0" reSnap
+
+    wrapProgram $out/bin/reSnap \
+      --suffix PATH : "${lib.makeBinPath finalAttrs.runtimeInputs}"
+  '';
+
+  installPhase = ''
+    runHook preInstall
+
+    install -D reSnap.sh $out/bin/reSnap
+
+    runHook postInstall
+  '';


Suggested change

postFixup = ''

substituteInPlace $out/bin/reSnap \

--replace-fail "\$0" reSnap

wrapProgram $out/bin/reSnap \

--suffix PATH : "${lib.makeBinPath finalAttrs.runtimeInputs}"

'';

installPhase = ''

runHook preInstall

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

runHook postInstall

'';

installPhase = ''

runHook preInstall

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

runHook postInstall

'';

postFixup = ''

substituteInPlace $out/bin/reSnap \

--replace-fail "\$0" reSnap

wrapProgram $out/bin/reSnap \

--suffix PATH : "${lib.makeBinPath finalAttrs.runtimeInputs}"

'';

Fixup is executed after install

github-actions bot added the 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` label Aug 14, 2024

ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Aug 14, 2024

eljamm suggested changes Aug 19, 2024

View reviewed changes

eljamm reviewed Aug 19, 2024

View reviewed changes

404Wolf force-pushed the add-resnap branch 3 times, most recently from 7e9084e to 72a9e5a Compare August 20, 2024 06:48

404Wolf changed the title ~~Add remarkable resnap~~ reSnap: init at 2.5.2 Aug 20, 2024

404Wolf force-pushed the add-resnap branch from 72a9e5a to 7ab9aa9 Compare August 20, 2024 06:59

eljamm suggested changes Aug 20, 2024

View reviewed changes

eljamm mentioned this pull request Aug 20, 2024

mailsy: init at 5.0.0 #332859

Merged

13 tasks

404Wolf force-pushed the add-resnap branch 3 times, most recently from da1552a to 62c1dcf Compare August 20, 2024 14:55

eljamm approved these changes Aug 20, 2024

View reviewed changes

eljamm added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Aug 20, 2024

SuperSandro2000 requested changes Aug 22, 2024

View reviewed changes

404Wolf force-pushed the add-resnap branch from 62c1dcf to 80a4a11 Compare August 22, 2024 15:20

404Wolf requested a review from emilylange as a code owner August 22, 2024 15:20

404Wolf force-pushed the add-resnap branch 2 times, most recently from 4771e0d to bbed8c0 Compare August 22, 2024 15:22

github-actions bot removed the 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` label Aug 22, 2024

emilylange removed their request for review August 22, 2024 22:51

404Wolf requested a review from SuperSandro2000 August 26, 2024 22:41

404Wolf force-pushed the add-resnap branch 3 times, most recently from 2f7d629 to 7ebf0f2 Compare August 27, 2024 16:14

eljamm reviewed Aug 27, 2024

View reviewed changes

SuperSandro2000 reviewed Aug 28, 2024

View reviewed changes

404Wolf force-pushed the add-resnap branch from 7ebf0f2 to 96b7668 Compare August 29, 2024 22:01

404Wolf requested a review from SuperSandro2000 August 29, 2024 22:04

404Wolf force-pushed the add-resnap branch from 96b7668 to 793b679 Compare August 29, 2024 22:04

reSnap: init at v2.5.2

7c3600a

404Wolf force-pushed the add-resnap branch from 793b679 to 7c3600a Compare August 30, 2024 14:13

wegank removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Aug 31, 2024

eljamm approved these changes Aug 31, 2024

View reviewed changes

eljamm added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Aug 31, 2024

lelgenio reviewed Sep 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

reSnap: init at 2.5.2 #334700

reSnap: init at 2.5.2 #334700

404Wolf commented Aug 14, 2024 •

edited

Loading

nixos-discourse commented Aug 19, 2024

eljamm Aug 19, 2024

eljamm Aug 19, 2024

eljamm Aug 19, 2024

eljamm Aug 19, 2024

eljamm Aug 19, 2024

404Wolf commented Aug 20, 2024

404Wolf commented Aug 20, 2024

eljamm Aug 20, 2024

404Wolf Aug 20, 2024

eljamm left a comment

SuperSandro2000 Aug 22, 2024

SuperSandro2000 Aug 22, 2024

SuperSandro2000 Aug 22, 2024

SuperSandro2000 Aug 22, 2024

404Wolf commented Aug 22, 2024

404Wolf commented Aug 26, 2024

eljamm commented Aug 27, 2024

404Wolf commented Aug 27, 2024 •

edited

Loading

eljamm commented Aug 27, 2024

eljamm Aug 27, 2024

eljamm Aug 27, 2024

SuperSandro2000 Aug 28, 2024

SuperSandro2000 Aug 28, 2024

SuperSandro2000 Aug 28, 2024

eljamm left a comment

404Wolf commented Sep 13, 2024

nixos-discourse commented Sep 24, 2024

lelgenio Sep 30, 2024

	substituteInPlace ./reSnap.sh --replace-quiet "\$0" "reSnap"
	substituteInPlace ./reSnap.sh --replace-fail "\$0" "reSnap"

reSnap: init at 2.5.2 #334700

Are you sure you want to change the base?

reSnap: init at 2.5.2 #334700

Conversation

404Wolf commented Aug 14, 2024 • edited Loading

Description of changes

Things done

nixos-discourse commented Aug 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

404Wolf commented Aug 20, 2024

404Wolf commented Aug 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eljamm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

404Wolf commented Aug 22, 2024

404Wolf commented Aug 26, 2024

eljamm commented Aug 27, 2024

404Wolf commented Aug 27, 2024 • edited Loading

eljamm commented Aug 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eljamm left a comment

Choose a reason for hiding this comment

404Wolf commented Sep 13, 2024

nixos-discourse commented Sep 24, 2024

Choose a reason for hiding this comment

404Wolf commented Aug 14, 2024 •

edited

Loading

404Wolf commented Aug 27, 2024 •

edited

Loading