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/printers: declarative configuration #55510

Merged
merged 1 commit into from Sep 3, 2019

Conversation

@florianjacob
Copy link
Contributor

florianjacob commented Feb 10, 2019

Motivation for this change

This saves one from going through the cups webinterface each time one sets up a new NixOS machine.
I created a half-stateful solution similar to mysql.ensureDatabases, which uses lpadmin etc. to create the configured printers and force them in the right configuration, without having to touch the deep parts of cups. Works quite well, at least for me.

Resolves #52776, supersedes #23684.

Tests, comments & proposals for improvements welcome.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@deliciouslytyped

This comment has been minimized.

Copy link
Contributor

deliciouslytyped commented Feb 10, 2019

A note: In my limited testing with lpadmin, if you add an existing printer again, any printer settings you have changed (for the given printer) seem to be reset. (so for example if reconfiguration calls lpadmin again)

Not a breaking thing, but possibly a bit annoying.
What is the behavior of "oneshot"?

When I poked at /etc/cups/printers.conf , printers had UUIDs. Its also possible the entry it just being overwritten with a new UUID so it only looked like the same entry, I haven't checked.

@florianjacob

This comment has been minimized.

Copy link
Contributor Author

florianjacob commented Feb 10, 2019

if you add an existing printer again, any printer settings you have changed (for the given printer) seem to be reset. (so for example if reconfiguration calls lpadmin again)

@deliciouslytyped that's actually my intention here, as the alternative would be that what's written in the nix config and the actual state of the printer can easily diverge, which I wanted to avoid as far as possible. I tried to improve the documentation in that this isn't a one-time thing, but will actually happen on each reboot or restart of cups. "oneshot" means that the lpadmin commands are run like a script on startup of cups. The main identifier is the printer name here; so subsequent lpadmin calls from the service will modify the existing printer.

@florianjacob

This comment has been minimized.

Copy link
Contributor Author

florianjacob commented Feb 10, 2019

Found the cups tests, rewriting them to use this instead of manual lpadmin commands…

@deliciouslytyped

This comment has been minimized.

Copy link
Contributor

deliciouslytyped commented Feb 10, 2019

Hm. Ok, I guess that's a reasonable decision, but I still feel some cognitive dissonance given that the stuff is basically mutable and on a first look doesn't appear touch any of the values in question.
TL;DR: I don't know what to do/think about it, just a little uneasy.

@florianjacob florianjacob force-pushed the florianjacob:declarative-printers branch from c2f0eaf to 30818c3 Feb 10, 2019
@florianjacob

This comment has been minimized.

Copy link
Contributor Author

florianjacob commented Feb 10, 2019

and on a first look doesn't appear touch any of the values in question

@deliciouslytyped Not sure what you're refering to here, could you explain what do you mean by that?

@florianjacob florianjacob force-pushed the florianjacob:declarative-printers branch 2 times, most recently from f102a13 to e22ab6d Feb 10, 2019
@florianjacob

This comment has been minimized.

Copy link
Contributor Author

florianjacob commented Feb 10, 2019

@GrahamcOfBorg test printing

@florianjacob

This comment has been minimized.

Copy link
Contributor Author

florianjacob commented Feb 10, 2019

Added support for socket activation, and added a socket activation version to the printing test to catch such errors.

nixos/tests/printing.nix Outdated Show resolved Hide resolved
nixos/tests/printing.nix Outdated Show resolved Hide resolved
$client->succeed("lpq -a") =~ /no entries/;
Machine::retry sub {
return 1 if $server->succeed("lpq -a") =~ /no entries/;
};

This comment has been minimized.

Copy link
@Mic92

Mic92 Feb 10, 2019

Contributor

Also here:

$server->waitUntilSucceeds("lpq -a | grep -q -E 'no entries'");
@florianjacob florianjacob force-pushed the florianjacob:declarative-printers branch from e22ab6d to a95a565 Feb 14, 2019
@florianjacob

This comment has been minimized.

Copy link
Contributor Author

florianjacob commented Feb 14, 2019

@Mic92 replaced all but one sleep with waitUntilSuceeds (no sure what I could match on to find out that service-based cups is actually ready and accepts printing jobs), and also find out about $machine->name() with which I could replace the my name = $machine->succeeds('hostname'); chomp $name; calls inside the testing function.

@florianjacob

This comment has been minimized.

Copy link
Contributor Author

florianjacob commented Feb 14, 2019

@GrahamcOfBorg test printing

@florianjacob florianjacob force-pushed the florianjacob:declarative-printers branch 3 times, most recently from 1e07fa8 to 863830e Feb 14, 2019
@florianjacob

This comment has been minimized.

Copy link
Contributor Author

florianjacob commented Aug 5, 2019

Damn, time for 19.09 already… But rebased, resolved conflicts, adressed more change requests. :)

@etu Thanks for your offer! 👍 In addition to doing another round of proofreading and testing with the new changes (and old ones like the waitUntilSucceeds changes which vanished somehow but are now recreated),
could you please take a look at my expanded description for #55510 (comment) , and try to reproduce #55510 (comment) ?
Also, if you know Perl better than I do, could you try to get rid of that Machine::retry thing I don't understand like suggested here? That would help alot, thank you!

@florianjacob florianjacob force-pushed the florianjacob:declarative-printers branch from b5407be to 63cfe71 Aug 7, 2019
@worldofpeace

This comment has been minimized.

Copy link
Member

worldofpeace commented Aug 28, 2019

Conflicts in the printing test is 01cd466, where it's default socket activated now.

@aaronjanse

This comment has been minimized.

Copy link
Member

aaronjanse commented Aug 28, 2019

In light of #53027, would we want to have paper size as a declarative option?

@etu

This comment has been minimized.

Copy link
Contributor

etu commented Aug 28, 2019

In light of #53027, would we want to have paper size as a declarative option?

That could be a very nice thing, at least for us that aren't in the US.

But at the moment I'm more worried if we get it in at all before the release.

Freeze in 10 days. https://discourse.nixos.org/t/nixos-19-09-feature-freeze/3707

@florianjacob florianjacob force-pushed the florianjacob:declarative-printers branch 2 times, most recently from e6f7bee to 325a69a Sep 1, 2019
@florianjacob

This comment has been minimized.

Copy link
Contributor Author

florianjacob commented Sep 1, 2019

Just resolved the conflicts, continuing to have both always-running and socket-activated cups in the tests because why not when I already wrote the code for both. But I don't cling to that.

@aaronjanse @etu thank you for reminding me, I already had the code to specify arbitrary ppd options laying around but forgot it was not yet included in this PR. Added those options, handy for working drivers with duplex print and the like, though they obviously don't help against broken drivers like in #53027.

Calling in @matthewbauer (👍 for making socket activation the new default) as new cups module maintainer for decision on what to do with this.

@florianjacob florianjacob force-pushed the florianjacob:declarative-printers branch from 325a69a to 0663b99 Sep 1, 2019
@florianjacob florianjacob force-pushed the florianjacob:declarative-printers branch from 0663b99 to 18a5d23 Sep 1, 2019
@florianjacob

This comment has been minimized.

Copy link
Contributor Author

florianjacob commented Sep 1, 2019

@GrahamcOfBorg test printing

@etu
etu approved these changes Sep 1, 2019
Copy link
Contributor

etu left a comment

Seems to work perfectly fine for me, even with setting page size etc :)

@etu etu requested review from Infinisil, worldofpeace and Mic92 Sep 1, 2019
Copy link
Member

worldofpeace left a comment

Let's go 😄

Thanks so much @florianjacob.

@florianjacob

This comment has been minimized.

Copy link
Contributor Author

florianjacob commented Sep 1, 2019

@etu @worldofpeace thanks! :)

To anybody lurking here and wanting to try this out, you can use this snippet here on stable 19.03 and then start to configure your printers declaratively right now:

{ ... }:
let
  declarativePrintersNixpkgsFJ = builtins.fetchGit {
    name = "florianjacob-nixpkgs-declarative-printers";
    url = https://github.com/florianjacob/nixpkgs/;
    ref = "declarative-printers";
    rev = "18a5d23b55d0b0d1bff6bfd971d471d3064011c2";
  };
in {
  imports = [
    "${declarativePrintersNixpkgsFJ}/nixos/modules/hardware/printers.nix"
  ];
}
@worldofpeace worldofpeace requested a review from disassembler Sep 2, 2019
@etu

This comment has been minimized.

Copy link
Contributor

etu commented Sep 2, 2019

If nobody comes with any real complaints about this before Friday (2019-09-06), I intend to merge it by then because it works perfectly fine in my tests.

cc @disassembler @Infinisil @Mic92

@Infinisil

This comment has been minimized.

Copy link
Member

Infinisil commented Sep 3, 2019

Looking good, let's go :)

@Infinisil Infinisil merged commit ad13ebe into NixOS:master Sep 3, 2019
14 checks passed
14 checks passed
tests.printing on x86_64-linux Timed out, unknown build status
Details
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
tests.printing on aarch64-linux Success
Details
@florianjacob florianjacob deleted the florianjacob:declarative-printers branch Sep 3, 2019
@florianjacob florianjacob restored the florianjacob:declarative-printers branch Sep 3, 2019
@florianjacob florianjacob deleted the florianjacob:declarative-printers branch Sep 11, 2019
in {
options = {
hardware.printers = {
ensureDefaultPrinter = mkOption {

This comment has been minimized.

Copy link
@edolstra

edolstra Oct 8, 2019

Member

Can we drop the "ensure" part? It seems redundant.

This comment has been minimized.

Copy link
@florianjacob

florianjacob Nov 18, 2019

Author Contributor

@edolstra You see a conceptual difference between ensurePrinters and ensureDefaultPrinter here, and therefore it's redundant for defaultPrinter but not for ensurePrinters, is that correct? I'm at your service. Maybe you could provide some general guidance on this, though?

I started to use those ensure prefixes to mark options that can only be enforced on a best effort basis on each service reload. In this concrete case, both ensurePrinters and ensureDefaultPrinter are translated into lpadmin command. I can't stop the user from modifying printers / change default printer, but the state will be restored. In case of ensurePrinters, I deliberately do not try to delete non-listed printers that the user might have added manually. Similar options in NixOS exist though which don't have that prefix.

There's a discussion going on about that, sparked off by similar mysql / postgresql options
I explained my point there as well.

EDIT: corrected link to discussion

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.