Conversation
YHNdnzj
left a comment
There was a problem hiding this comment.
Need to be removed from documentation too?
|
Reverting a feature is unfortunate, but in this particular case I think it's the right thing to do. The feature is too much of a footgun right now, and has caused real damage. I agree that the whole design choices need to be reconsidered for the next version. |
|
I don't know how realistic/wanted this is, but maybe defaulting And specifically for the current issue: maybe calling it |
This effectively reverts 81a1838. Note that the actual implementation was retained, but there is no way to invoke it once the commandline switch is removed. I expect that the switch will be restored in a slightly different form, so it doesn't make sense to create churn by removing all the code. As discussed in systemd#33349, --purge provides a footgun for users by allowing them to easily remove contents of /home. At least one user has done this, which is one too many. Let's disable the command before this hits stable distributions and more users have a chance to destroy their data. Closes systemd#33349.
31538e6 to
8c09661
Compare
Arrghs, I had an unsaved change. The docs are removed now too. |
I don't think we can do this easily, because it'd break backwards compat. We could do that conditionally with |
|
I upgraded the green label because comments have been addressed, but let's wait for a review from @bluca too. |
|
What about an isatty check? Maybe even imply dry run when run in an interactive shell. |
Dunno, such things are always very fragile. You run a command in a script and it works, then copy it into the terminal and it doesn't? Or run the script from a terminal and it's broken? But pipe it to less and it works again. I think it's fine when we enable/disable coloring and output details based on such checks, but I wouldn't apply it to basic (and dangerous) rules of behaviour. |
|
Let's not overreact over social media drama from well-known trolls. This is already in use in Debian/Ubuntu packages (edit: here https://salsa.debian.org/debian/debhelper/-/blob/main/autoscripts/postrm-init-tmpfiles-purge ). It's fine to make this act only on input (stdin or explicit config file), and add another flag like --factor-reset, perhaps not documented, for the global behaviour. None of this is ever intended to be ran manually anyway, there's no documentation that I am aware of that even hints of doing that, it's for timed services and packaging scriptlets. Changing documentation is of course also fine to do, if you like. It is true that tmpfiles.d has long since not been only about temporary files, and for years has taken care of /etc/ for example, and the docs could use some freshing up. |
|
My reasoning is not based on social media reports, but on the github issue and analysis of the interface.
This sounds dangerous to use in an automatic fashion. I wouldn't unleash this on my users. But actually this use is compatible with the removal, since the script checks for
I think that's something to discuss before reenabling this option.
I guess the intent was not, but our intent is that important. What matters is that users clearly are using this in unintended ways. And the tools is intended to be used in scripts and various special ways. The tool has a user interface and --help and is a normal user-level tool, just not in $PATH. Do you have any actual comments on the patch as written? Or a suggestion how to resolve the issue in a different way? But all that must be actionable now. We have a stable release that in principle we consider appropriate for users and I want to remove this footgun before another user looses data. |
Sure, but if it wasn't for a couple of trolls on social media, nobody would have cared in the first place.
It's not dangerous at all, as it wired up to run only on the specified input. The purpose of the purge operation is to remove any trace that a package might have left behind, and that's exactly what it does. Removal is not compatible with it, as it regresses: leftovers will be left around. Sorry, such a regression not acceptable.
There's no need to rush anything. The proposal to change the default to act only on explicit input is compatible with current use so it's acceptable to me, and it will shut up the trolls on social media too.
Personally I am actually very, very skeptical actual users are really doing anything of the sort. Even the bug report I find highly dubious: why would anybody who has never heard of this tool and has never used it, think of running it just to remove something from /var/cache as reported, and with this option instead of the more obvious clean/remove on top of that? There is no documentation nor hint nor help nor manpage anywhere suggesting to do that. There's no mention of /var/cache anywhere that I can find. I find it extremely weird. This all started on IRC first and then moved to social media, so I wouldn't be surprised if what really happened is that someone read NEWS, found the entry and decided to have a bit of fun.
Yes, as already mentioned, Lennart's suggestion sound fine to me: make the switch act only on the provided input. This is actionable now too, and doesn't sound too hard. Updating the documentation is also actionable now. |
It's concerning that people raising valid concerns are called trolls.
This reminds me of #25269 (and I thought nothing could top it honestly). In the end it was of course a badly designed feature with sloppy UX that someone else had to fix as usual. @keszybz as far as I can see the majority are fine with merging this so I wonder why it's still being discussed? Whoever wants to keep it downstream can apply patches there. |
|
the --purge stuff is about ownership of files. I.e. that stuff "owned" by some specific package can be removed nicely when the package goes away. Given we know this usecase very clearly I am pretty sure we should just require that a config snippet is specified on the invocation (either by filename on cmdline or via data on stdin), and if it isn't we refuse to do anything, because it simply makes no sense to "delete everything owned", because that necessarily destroys the whole system. I think it's quite a high bar against accidental invocation if we require people to specify valid tmpfiles.d/ drop-in filenames or valid definitions on stdin before we do anything. Hence I think that should really be enough as a protection. |
|
My alternative fix: #33383 ptal. |
|
replaced by #33383 |
This effectively reverts 81a1838. Note that the actual implementation was retained, but there is no way to invoke it once the commandline switch is removed. I expect that the switch will be restored in a slightly different form, so it doesn't make sense to create churn by removing all the code.
As discussed in #33349, --purge provides a footgun for users by allowing them to easily remove contents of /home. At least one user has done this, which is one too many. Let's disable the command before this hits stable distributions and more users have a chance to destroy their data.
Closes #33349.