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/calibre-web: customize data directory #350617

Merged
merged 1 commit into from
Jan 13, 2025

Conversation

aymanbagabas
Copy link
Contributor

This commit adds the ability to customize the data directory where Calibre-Web stores its data. It uses systemd tmpfiles to create the directory with the correct permissions.

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.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
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

This commit adds the ability to customize the data directory where
Calibre-Web stores its data. It uses systemd tmpfiles to create the
directory with the correct permissions.
@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 Oct 23, 2024
@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 Oct 23, 2024
Copy link
Member

@FliegendeWurst FliegendeWurst left a comment

Choose a reason for hiding this comment

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

Seems reasonable

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Nov 1, 2024
@FliegendeWurst FliegendeWurst changed the title calibre-web: customize data directory nixos/calibre-web: customize data directory Jan 13, 2025
@FliegendeWurst FliegendeWurst merged commit e85ca75 into NixOS:master Jan 13, 2025
40 checks passed
aspauldingcode pushed a commit to aspauldingcode/nixpkgs that referenced this pull request Jan 16, 2025
@zehauser
Copy link

Wasn't this a breaking change?

@FliegendeWurst
Copy link
Member

If you changed the value of the option, yes.

@zehauser
Copy link

zehauser commented Jan 21, 2025

I did indeed set the option. 🙂 This change caused calibre-web to crash-loop with a confusing error.

Do we not normally try to avoid needlessly breaking changes in NixOS modules? I'm not super familiar with NixOS community standards yet, though I have been using NixOS for a year or two now and regularly updating (I'm on nixos-unstable) and have not seen any other cases like this yet. I have seen options get deprecated or renamed with a clear eval-time warning or error message.

This breakage wasn't an edge case — we just changed the meaning of the option unconditionally, so that anyone who set it would be broken (at runtime). Wouldn't it have been better to deprecate and create a new option? Or even to try to do something clever like try to recognize an absolute path (the new semantics) vs a relative path (the old semantics)?

I'm not trying to criticize anybody here — I'd genuinely like to learn.

@FliegendeWurst
Copy link
Member

You are right, this should have included an assertion that the provided value is an absolute path, and a changelog entry.
Check out #375539 for a different approach, where I restore the old behaviour whilst also allowing a fully customized data directory.

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 12.approvals: 1 This PR was reviewed and approved by one reputable person
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants