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/cupsd: new option for specifying printers.conf declaratively. #23684

Closed

Conversation

jraygauthier
Copy link
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

@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
Member Author

Would also be a candidate for release-17.03.

Copy link
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?

description = ''
Contents of the printers configuration file of the CUPS daemon
(<filename>printers.conf</filename>). When left null, cups will use
the (<filename>/etc/cupsd/printers.conf</filename>) as the configuration
Copy link
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
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
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
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
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
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
Member Author

@jraygauthier
Copy link
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
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
Copy link
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
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
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).

@@ -305,6 +321,16 @@ in
[ -L "$i" ] && rm "$i"
Copy link
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
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/*.

@c0bw3b
Copy link
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants