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

autobrr: init at 1.36.0 #287593

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

autobrr: init at 1.36.0 #287593

wants to merge 3 commits into from

Conversation

av-gal
Copy link

@av-gal av-gal commented Feb 9, 2024

Description of changes

Reopening #283389. Closes #224560.

Things done

  • Built on platform(s)
    • 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
  • Tested, as applicable:
  • 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
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 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
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

Copy link
Member

@dotlambda dotlambda left a comment

Choose a reason for hiding this comment

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

Three commits please:

  • maintainers: add av-gal
  • autobrr: init at 1.36.0
  • nixos/autobrr: init

maintainers/maintainer-list.nix Outdated Show resolved Hide resolved
nixos/doc/manual/release-notes/rl-2405.section.md Outdated Show resolved Hide resolved
nixos/modules/module-list.nix Outdated Show resolved Hide resolved
pkgs/by-name/au/autobrr/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/au/autobrr/package.nix Show resolved Hide resolved
pkgs/by-name/au/autobrr/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/au/autobrr/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/au/autobrr/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/au/autobrr/package.nix Outdated Show resolved Hide resolved
@dotlambda dotlambda changed the title Autobrr: add package/module/test for autobrr, attempt #2 autobrr: init at 1.36.0 Feb 9, 2024
av-gal

This comment was marked as duplicate.

@av-gal av-gal requested a review from dotlambda February 9, 2024 22:57
@av-gal
Copy link
Author

av-gal commented Feb 9, 2024

I think I've addressed all remaining concerns. @ambroisie please let me know if I've missed anything from your original review.

Copy link
Member

@dotlambda dotlambda left a comment

Choose a reason for hiding this comment

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

There should be a settings option to create config.toml. See NixOS/rfcs#42.

DynamicUser = true;
StateDirectory = "autobrr";
StateDirectoryMode = "0700";
ExecStart = "${pkgs.autobrr}/bin/autobrr --config '${WorkingDirectory}'";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ExecStart = "${pkgs.autobrr}/bin/autobrr --config '${WorkingDirectory}'";
ExecStart = "${lib.getExe pkgs.autobrr} --config '${WorkingDirectory}'";


vendorHash = "sha256-SkwSKFEZAmjVnaSowIbrdH667vB5WqNrPuRs/Yh6BLc=";

preBuild = "cp -r ${autobrr-web}/* web/dist";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
preBuild = "cp -r ${autobrr-web}/* web/dist";
preBuild = ''
cp -r ${autobrr-web}/* web/dist
'';

DynamicUser = true;
StateDirectory = "autobrr";
StateDirectoryMode = "0700";
ExecStart = "${pkgs.autobrr}/bin/autobrr --config '${WorkingDirectory}'";
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is WorkingDirectory defined?

Copy link
Contributor

Choose a reason for hiding this comment

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

And please use lib.getExe:

Suggested change
ExecStart = "${pkgs.autobrr}/bin/autobrr --config '${WorkingDirectory}'";
ExecStart = "${lib.getExe pkgs.autobrr} --config '${WorkingDirectory}'";

Comment on lines +21 to +46
autobrr-web = stdenvNoCC.mkDerivation {
pname = "${pname}-web";
inherit src version;

nativeBuildInputs = [
nodePackages.pnpm
cacert
];

buildPhase = ''
runHook preBuild

export HOME=$(mktemp -d)
pnpm --dir web install
pnpm --dir web run build

mv web/dist $out

runHook postBuild
'';

dontInstall = true;

outputHashMode = "recursive";
outputHash = "sha256-IOrW26Nq+9PYWlzUSpPBfkv3jzs5VlfF0JVFaUCDMmw=";
};
Copy link
Contributor

Choose a reason for hiding this comment

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

So at least we're not using an FOD to store the pnpm dependencies. But I still worry that the output is not bit-for-bit reproducible.

My recommendation is to:

  1. Try to use pnpm-export and use buildNpmPackage or mkYarnPackage.
  2. If pnpm-export doesn't work, just generate a lock file manually.

In either case, this would mean vendoring the lock file to the repository.

Copy link
Author

Choose a reason for hiding this comment

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

I believe this derivation should be bit-for-bit reproducible. Different versions of pnpm might have different store or cache formats, but each module in node_modules will be consistent with its hash in pnpm-lock.yaml. dist just contains the final bundle created by Vite. Why would that change because the dependencies are grabbed with a different package manager?

Copy link
Member

Choose a reason for hiding this comment

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

What makes you believe the output of pnpm --dir web run build is reproducible? Do TypeScript and Vite claim to be reproducible?

Copy link
Member

Choose a reason for hiding this comment

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

error: build of '/nix/store/cfcl0gpb9p5nnphw25g9mc6hifjpf3wl-autobrr-web-1.36.0.drv' on 'ssh://nix@willard' failed: hash mismatch in fixed-output derivation '/nix/store/cfcl0gpb9p5nnphw25g9mc6hifjpf3wl-autobrr-web-1.36.0.drv':
         specified: sha256-IOrW26Nq+9PYWlzUSpPBfkv3jzs5VlfF0JVFaUCDMmw=
            got:    sha256-SkwSKFEZAmjVnaSowIbrdH667vB5WqNrPuRs/Yh6BLc=

Copy link
Author

Choose a reason for hiding this comment

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

pnpm --dir web run build just runs tsc && vite build, as specified in package.json. Neither of these claim to be reproducible build tools (I don't think), but I believe that buildNpmPackage or mkYarnPackage would execute those commands in the same way. Please correct me if I'm wrong.

Copy link
Author

Choose a reason for hiding this comment

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

May I ask if you did anything to cause the hash mismatch? Or did you just try to build it on your machine

Copy link
Member

Choose a reason for hiding this comment

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

The latter.

Neither of these claim to be reproducible build tools (I don't think), but I believe that buildNpmPackage or mkYarnPackage would execute those commands in the same way.

Yes but those aren't used in FODs.


vendorHash = "sha256-SkwSKFEZAmjVnaSowIbrdH667vB5WqNrPuRs/Yh6BLc=";

preBuild = "cp -r ${autobrr-web}/* web/dist";
Copy link
Contributor

Choose a reason for hiding this comment

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

For easy diffs, I'd use a multi-line string:

Suggested change
preBuild = "cp -r ${autobrr-web}/* web/dist";
preBuild = ''
cp -r ${autobrr-web}/* web/dist
'';

@@ -0,0 +1,70 @@
{ config, pkgs, lib, ... }:

with lib;
Copy link
Member

Choose a reason for hiding this comment

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

consider inherits where you find yourself using lib.* excessively.
this nix.dev page on best practices with with may be helpful
an issue (#208242) has been made to track inappropriate uses of with such as this.

openFirewall = mkOption {
type = types.bool;
default = false;
description = lib.mdDoc "Open ports in the firewall for the Autobrr web interface.";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
description = lib.mdDoc "Open ports in the firewall for the Autobrr web interface.";
description = "Open ports in the firewall for the Autobrr web interface.";

lib.mdDoc is now just an alias and can be safely removed everywhere.
see d36f950 and #237557

@zze0s
Copy link

zze0s commented Feb 14, 2024

We just released v1.38.0 so might be a good idea to bump the version in this pr 😄

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.

Package request: autobrr
7 participants