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: add package/module/test for autobrr #283389

Merged
merged 0 commits into from Feb 9, 2024
Merged

Conversation

av-gal
Copy link

@av-gal av-gal commented Jan 24, 2024

Description of changes

Adds autobrr to nixpkgs and as a NixOS module. I based this off of borgstad's pull request #239107, attempting to implement the reviewers' feedback. Closes #224560.

Many thanks in advance for your feedback :).

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
Contributor

@ambroisie ambroisie left a comment

Choose a reason for hiding this comment

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

Please rebase and squash your commits to the following:

  • maintainers: add av-gal
  • maintainers: add borgstad (does @borgstad actually want to be added?)
  • autobrr: init at 1.35.1
  • nixos/autobrr: init module
  • nixos/autobrr: init test

The release note can either go in the module commit, or by itself.

nixos/modules/misc/ids.nix Outdated Show resolved Hide resolved
@@ -666,6 +666,7 @@ in
rstudio-server = 324;
localtimed = 325;
automatic-timezoned = 326;
autobrr = 327;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same note for gid.

nixos/modules/services/misc/autobrr.nix Outdated Show resolved Hide resolved
nixos/modules/services/misc/autobrr.nix Outdated Show resolved Hide resolved
nixos/modules/services/misc/autobrr.nix Outdated Show resolved Hide resolved
nixos/tests/autobrr.nix Outdated Show resolved Hide resolved
@@ -0,0 +1,96 @@
{ lib, cacert, esbuild, jq, moreutils, buildGoModule, fetchFromGitHub, stdenvNoCC, nodePackages, typescript }:
Copy link
Contributor

Choose a reason for hiding this comment

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

For such a long list of argument, it is customary to put one argument per line for easier diff reviewing.

};

pnpm-deps = stdenvNoCC.mkDerivation {

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

Comment on lines 13 to 49
pnpm-deps = stdenvNoCC.mkDerivation {

pname = "${pname}-npm-deps";
inherit src version;

nativeBuildInputs = [
jq
moreutils
nodePackages.pnpm
cacert
];

dontBuild = true; #this is just a download
dontCheck = true;
installPhase = ''
runHook preInstall

export HOME=$(mktemp -d)
pnpm config set store-dir $out
# use --ignore-script and --no-optional to avoid downloading binaries
# use --frozen-lockfile to avoid checking git deps
pnpm --dir web fetch

# Remove timestamp and sort the json files
rm -rf $out/v3/tmp
for f in $(find $out -name "*.json"); do
sed -i -E -e 's/"checkedAt":[0-9]+,//g' $f
jq --sort-keys . $f | sponge $f
done

runHook postInstall
'';

dontFixup = true;
outputHashMode = "recursive";
outputHash = "sha256-babKc60nuu+rfI5nnp7efvo4Z9Iz93CkH4qg7l3rRxA=";
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Until #231513 is resolved, we don't have a good user-story for pnpm in nixpkgs.

If upstream has a pre-built frontend packaged, I would suggest you use that for now.

Otherwise I would suggest manually generating a package-lock.json/yarn.lock and make use of buildNpmPackage/mkYarnPackage for now.

The reason being that this FOD will break when pnpm changes the format for its store...

Comment on lines 83 to 88
preBuild = ''
export HOME=$(mktemp -d)
pnpm config set store-dir ${pnpm-deps}
pnpm --dir web --offline install
pnpm --dir web run build
'';
Copy link
Contributor

Choose a reason for hiding this comment

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

See my other comment about pnpm.

@av-gal av-gal closed this Feb 9, 2024
@thiagokokada thiagokokada merged commit ac8579a into NixOS:master Feb 9, 2024
4 of 6 checks passed
@winterqt
Copy link
Member

winterqt commented Feb 9, 2024

I think @av-gal got this PR into a weird state. It was force-pushed to a commit that... someone (not @thiagokokada) merged 36 minutes ago? (#287469)

So that must be why GitHub marked this as merged. No idea why Thiago would be marked as the one who merged this PR, though, as he didn't merge the other PR GitHub thinks this PR is. Perhaps it's a race condition, as Thiago has been merging a bunch of PRs in the past few minutes.

@winterqt
Copy link
Member

winterqt commented Feb 9, 2024

@ambroisie Did GitHub delete your comment on its own, or was that you? 😛

@av-gal av-gal mentioned this pull request Feb 9, 2024
13 tasks
@av-gal
Copy link
Author

av-gal commented Feb 9, 2024

@winterqt I was trying to clean up my commit history by force-pushing to a more recent commit, then adding my changes on top of that. But it seems like GitHub marked this as merged after my first force-push, causing this PR to be locked forever. There's a new PR for this branch at #287593.

@ambroisie
Copy link
Contributor

@winterqt I figured out that the PR was empty, @thiagokokada was innocent (sorry for the pings...), so I deleted my comment.

@ambroisie
Copy link
Contributor

@av-gal in the future, just (force) push your (rebased) changes directly, GitHub handles it well enough

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment