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

nixos/paperless: use toShellVars for paperless-manage #242275

Merged
merged 2 commits into from
Jul 25, 2023

Conversation

Atemu
Copy link
Member

@Atemu Atemu commented Jul 8, 2023

The homebrewed snippet didn't escape vars properly which is an issue because PAPERLESS_OCR_USER_ARGS requires a JSON string. This also meant a discrepancy between the services' env vars and paperless-manage's.

Just use the correctly functioning library function for this instead.

Description of changes
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 23.11 Release Notes (or backporting 23.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
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Jul 8, 2023
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Jul 8, 2023
The homebrewed snippet didn't escape vars properly which is an issue because
PAPERLESS_OCR_USER_ARGS requires a JSON string. This also meant a discrepancy
between the services' env vars and paperless-manage's.

Just use the correctly functioning library function for this instead.
@Atemu Atemu force-pushed the nixos/paperless-manage-toShellVars branch from e6928e8 to 44f637a Compare July 8, 2023 14:57
@leona-ya
Copy link
Member

leona-ya commented Jul 9, 2023

In #242084 is some discussion about the same issue. We have to decide which approach we want to use.
I like the approach here more, but I think we should include the added docs from the PR (not strong opinion)

@erikarvstedt
Copy link
Member

should include the added docs from

The env of the manage script must equal the env of the services. This is the default, expected case and thus needs no extra comment.

@Atemu
Copy link
Member Author

Atemu commented Jul 9, 2023

The doc from the other PR is only necessary because it still doesn't quote shell vars correctly. toShellVars does.

The hint about using toJSON would be a handy addition though.

@erikarvstedt
Copy link
Member

erikarvstedt commented Jul 9, 2023

The hint about using toJSON would be a handy addition though.

Ah, that's what @leona-ya meant. Yes, this should be included. Here's the version from my fixup.

@leona-ya
Copy link
Member

leona-ya commented Jul 9, 2023

Oh yes, that's what I meant. I'm sorry that was imprecise

Co-authored-by: Christian Theune <[email protected]>
Co-authored-by: Erik Arvstedt <[email protected]>
@Atemu
Copy link
Member Author

Atemu commented Jul 10, 2023

Any further objections?

@erikarvstedt
Copy link
Member

erikarvstedt commented Jul 10, 2023

@Atemu, tbh, I'd prefer to avoid toShellVars's pessimized loop over env, given Nix' performance issues.
Alternative:

manage = pkgs.writeShellScript "manage" ''
   ${lib.concatStringsSep "\n" (mapAttrsToList (name: val: "export ${name}=${escapeShellArg val}") env)}
   exec ${pkg}/bin/paperless-ngx "$@"
'';

This avoids:

  • The isValidPosixName regex check. All var names that paperless accepts are POSIX compatible.
  • The value type checks. These are always strings. (We should change extraConfig.type to attrsOf str).

Weak preference, so I'm fine with ACKing this PR as it is.

@erikarvstedt
Copy link
Member

erikarvstedt commented Jul 12, 2023

We need the verdict of a senior Nixer. @mweinelt, can you help us out?

Copy link
Member

@erikarvstedt erikarvstedt left a comment

Choose a reason for hiding this comment

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

Let's get this merged.

@mweinelt
Copy link
Member

We need the verdict of a senior Nixer. @mweinelt, can you help us out?

Slight preference for easier to read code in this case.

@github-actions
Copy link
Contributor

Successfully created backport PR for release-23.05:

@Atemu Atemu deleted the nixos/paperless-manage-toShellVars branch July 25, 2023 12:08
@Atemu
Copy link
Member Author

Atemu commented Jul 25, 2023

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants