dpkg provider cleanup #4185
dpkg provider cleanup #4185
Conversation
|
@chef/client-core more review |
|
travis failure is unrelated: chef/chef-server#624 |
|
this looks just peachy 👍 |
| def run_noninteractive(command) | ||
| shell_out_with_timeout!(command, :env => { "DEBIAN_FRONTEND" => "noninteractive", "LC_ALL" => nil }) | ||
| shell_out_with_timeout!(command, :env => { "DEBIAN_FRONTEND" => "noninteractive" }) |
There was a problem hiding this comment.
why was it originally left as nil? was it a parsing issue on systems with foreign character sets?
There was a problem hiding this comment.
It seems to have been in response to this bug:
https://tickets.opscode.com/browse/CHEF-281
The root cause of that bug however was using the "en_US" locale which didn't exist on systems without en_US installed. We've got complicated workarounds now which favor all kinds of different en_US locales and then fallback to to the 'C' locale as default.
The commit which introduced it just says:
Author: Diego Algorta [email protected]
Date: Fri Aug 14 19:54:31 2009 -0300
Make sure all Package providers run with the system's locale and not C locale.
Surely people want they're packages to corrently detect the system's
locale when configuring themselves.
And I disagree completely with that logic. If that logic was right, then if you installed a package with your machine set to en_US.UTF-8 and then changed your locale to de_DE.UTF-8 you would have to reinstall all your packages to get the "locale detection on package installation" right. Packages can't have a requirement to 'detect' the locale on installation or else locale switching would be completely broken.
There was a problem hiding this comment.
At some point we also tore it out of all the rest of the package providers, including the apt provider so that now they call shell_out_with_timeout! which calls shell_out! and this is the only one that still overrides its LC_ALL setting. Pretty certain this line of code is just a cargo cult now. If it wasn't then the changes introduced in the apt provider would have broken the world a long time ago.
|
👍 |
- :update and :install are now treated the same way and throw the same exceptions - :remove and :purge don't require the source at all, so don't do any checking on that - fix some convoluted side-effecty logic in load_current_resource - load_current_resource now correctly gets the dpkg state on :remove and :purge when the file does not exist (pretty sure the old logic did not) - fixed the FIXME about using en_US.UTF-8 (the default for shell_out!) - just use shell_out! to throw exceptions - clean up all the specs and remove all the instance vars from the code
lamont-granquist
force-pushed
the
lcg/dpkg-refactor
branch
from
abf20f4 to
1b95e4d
Compare
dpkg provider cleanup
same exceptions
any checking on that
:remove and :purge when the file does not exist (pretty sure
the old logic did not)