Skip to content

nixos/cupsd: new option for specifying printers.conf declaratively.#23684

Closed
jraygauthier wants to merge 1 commit intoNixOS:masterfrom
jraygauthier:jrg/cupsd_printersconf_option
Closed

nixos/cupsd: new option for specifying printers.conf declaratively.#23684
jraygauthier wants to merge 1 commit intoNixOS:masterfrom
jraygauthier:jrg/cupsd_printersconf_option

Conversation

@jraygauthier
Copy link
Copy Markdown
Member

Motivation for this change

To allow for declarative specification of cups printers.

Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • 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/)
  • Fits CONTRIBUTING.md.

@mention-bot
Copy link
Copy Markdown

@jraygauthier, thanks for your PR! By analyzing the history of the files in this pull request, we identified @edolstra, @abbradar and @urkud to be potential reviewers.

@jraygauthier
Copy link
Copy Markdown
Member Author

Would also be a candidate for release-17.03.

Copy link
Copy Markdown
Member

@abbradar abbradar left a comment

Choose a reason for hiding this comment

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

We generally don't add features to the release after branch-off. Do you think it's highly desirable?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe /etc/cups/ was meant here? Also CUPS will use this path anyway, it's just that the file will or won't be there.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Disregard the second point, I forgot how our CUPS works.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

ah yes, cups was meant. Will fix the doc. As to the behavior, as with other configuration files, as soon as the configuration is modified through the web interface, cups will move printers.oonf aside to printers.conf.0. Otherwise, cups will use the declarative file. This seems to be consistent with
the behavior of other configurations files.

It is indeed desirable to me, not sure about others. I can live with rebasing the changeset over 17.03 though as long as it is available in the next release (17.09?).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@globin, how do you feel about porting this to 17.03? This is a feature but seems a very small one.

@jraygauthier jraygauthier force-pushed the jrg/cupsd_printersconf_option branch 2 times, most recently from 378ed78 to 52fee6a Compare March 10, 2017 13:48
@jraygauthier
Copy link
Copy Markdown
Member Author

I just noticed that cups will also move aside the configuration when stopped and the configuration is present. It makes this only semi useful.

Do you think it would be a good that when declarative printers configuration is requested (non null),
we replace the non declarative one on preStart so that the declarative configuration take precedence?

@abbradar
Copy link
Copy Markdown
Member

abbradar commented Mar 10, 2017

Seems fair -- we already do this in several other modules. EDIT: Please mention it in the docs!

@jraygauthier
Copy link
Copy Markdown
Member Author

@jraygauthier jraygauthier force-pushed the jrg/cupsd_printersconf_option branch from 52fee6a to c18acb9 Compare March 15, 2017 03:10
@jraygauthier
Copy link
Copy Markdown
Member Author

An improved version which replaces the cups modified version with the declarative version when specified. About the documentation, do you have a suggestion as to where to bring this up? I had a look at nixos manual and there seems to be no section about printer configuration. Were you rather referring to the release notes?

@jraygauthier
Copy link
Copy Markdown
Member Author

I am currently investigating whether it would be possible to symlink instead of copy. It would provide us with a way of knowing that the current file already is the declarative version and avoid the operation completly.

@jraygauthier jraygauthier force-pushed the jrg/cupsd_printersconf_option branch from c18acb9 to 04e0838 Compare March 15, 2017 03:46
@jraygauthier
Copy link
Copy Markdown
Member Author

This latest version with the symlink seems to also play well with cups and its behavior is closer to what's usually done by nixos. It will even backup a previous non-declarative version of the configuration to cups's standard backup location.

ln -s --backup=simple --suffix='.O' \
"${rootdir}/etc/cups/printers.conf" "/var/lib/cups/printers.conf"
fi

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So the difference to what is done in the following lines is that we do a backup here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Not exactly. The difference that matters is that for this particular file we now systematically / always do the linking even when the file exist (in which case we do a backup).

@joachifm joachifm added 0.kind: enhancement Add something new or improve an existing system. 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: printing Drivers, CUPS & Co. labels May 21, 2017
@@ -305,6 +321,16 @@ in
[ -L "$i" ] && rm "$i"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Make that rm -- "$i"

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This line is not part of the changeset. Is there any reason you mention this as part of this PR?

according to https://ss64.com/bash/rm.html:

The rm command accepts the -- option which will cause it to stop processing flag options from that point forward.
This allows the removal of file names that begin with a dash -.
rm -- -filename
Alternatively use an absolute or relative path reference.
rm /home/user/-filename
rm ./-filename

As I understand it, the -- add no value here as i will always be filled with an absolute path of the form /var/lib/cups/*.

@veprbl veprbl added the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 28, 2018
@c0bw3b
Copy link
Copy Markdown
Contributor

c0bw3b commented May 21, 2019

Stalled.
Newer work in #55510

@c0bw3b c0bw3b closed this May 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

0.kind: enhancement Add something new or improve an existing system. 2.status: merge conflict This PR has merge conflicts with the target branch 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: printing Drivers, CUPS & Co.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants