Fix FileUtils.rm_rf to only mask exceptions about non existent files

[deivid-rodriguez]

This is one idea to fix #57.

I think this is a bug, so I didn't care about backwards compatibility. In my opinion, any code forcefully removing a folder expects the folder to be removed, and if not, it will fail later on, like it happened in bundler.

[deivid-rodriguez]

Apparently my changes broke the JRuby CI, I'll have a look.

[deivid-rodriguez]

I'm thinking maybe an alternative fix would be to try give the folder executable permissions before trying to remove it?

[deivid-rodriguez]

Alright, I changed the approach to automatically try fixing permissions in this particular case, because:

• It should be 100% backwards compatible.
• It's an approach already used for other edge cases.

[deivid-rodriguez]

For JRuby I had to add some special code, because it raises a different error from CRuby. Issue is already reported upstream: jruby/jruby#5617.

[deivid-rodriguez]

Alright, I changed my mind again on this.

I think fileutils should just be a thin wrapper on top of standard shell utils, so FileUtils.rm_rf should behave just like rm -rf and not try to fix permissions at all.

The bug reported still stands, and I think my original approach would be best, so I'll go back to that.

[deivid-rodriguez]

I reencountered an issue related to this.

I was debugging a dependabot spec failure on Bundler 1.x + Ruby 3.1, and it was very hard to debug because the underlying issue ended up being a FileUtils.rm_rf call that was failing silently due to the vendored version of fileutils provided by Bundler 1.x not supporting the separation of keyword and positional arguments in Ruby 3.

So a

ArgumentError: wrong number of arguments (given 2, expected 1)

was being raised, but then swallowed, and the error was happening later on on a completely unrelated place, due to the spec assuming removal had happened successfully.

So, I changed this PR to only mask the errors about non existent files when the force flag is given.

This matches exactly rm shell behavior and man rm documentation, which states

COMPATIBILITY

The rm utility differs from historical implementations in that the -f option only masks attempts to remove non-existent files instead of masking a large variety of errors.

[deivid-rodriguez]

Hello @nobu @hsbt, I'd like some feedback about this if possible. Thank you 🙏.

[deivid-rodriguez]

Sorry for the ping, this patch still has some issues. I'll ping you back once it's ready!

[deivid-rodriguez]

Ok, so I run this code with RubyGems tests and it detected a couple of issues in the tests. See ruby/rubygems#5475. I also run this changeset against ruby/ruby CI at ruby/ruby#5811 and everything is green after fixing Rubygems test issues.

So, this change could break some tests, but in my opinion, it will be for good because it will help detect issues. Given that, I see a couple of options:

• Ship this as is and call it a bug fix.
• Ship this as fileutils 2.0.0.
• Make this really backwards compatible by adding a separate value for this behavior, for example, force = :strict or something.

I haven't get any feedback at all so far, so I will wait a few days and then bring it to ruby-core.

[eregon]

Looks good to me, indeed swallowing exceptions in gems/libraries leads to nasty hard-to-debug bugs and I think hurts more than help.

[deivid-rodriguez]

I found a better alternative to be able to see the original exception that does not require any changes in fileutils: using FileUtils.remove_entry(path) if File.exist?(path). That's equivalent to FileUtils.rm_rf(path) but without the exception swallowing I'm reporting here.

So I'm closing this PR given it has been ignored for a long time.

[deivid-rodriguez]

I realized this ensure might have been intentional.

The current behavior is, even if an exception happen, still try to remove the given folder (and most likely get a Errno::ENOTEMPTY exception, because the original exception probably prevented some of the files inside to be removed). In that case, the original exception is still reported as the exception cause.

In Bundler, I was not seeing the original error because we're failing to report the cause if it exists in a couple of places, but I will fix this in Bundler.

This is all to say that, if the behavior I proposed is to be implemented, we might still want to restore this ensure.

[eregon]

One issue with FileUtils.remove_entry is it's not really an intuitive name, and probably far more people will run into the shortcomings of FileUtils.rm_rf(path).

[deivid-rodriguez]

Yeah, when I said "better alternative" I meant to what I'm doing now (rechecking whether the folder still exists after FileUtils.rm_rf, and raise an exception "guessing" the actual culprit). But fixing FileUtils.rm_rf would be best in my opinion.

[codiophile]

I do agree that rm_rf is the most intuitive name and the method that most people will be using. I also believe that most people will assume that rm_rf will behave like rm -rf, which will cause issues further down the line, because rm_rf is silently failing, causing issues elsewhere.

It seems like it would be more useful, if it ignored does not exist errors (and warned about ~ not working, which is what got me), and threw other errors, as most people would expect that the thing they are trying to delete will be gone after it's been deleted.

I wonder why this issue has went ignored, as it seems like a high value thing to fix. Sure :force ignoring all errors is a feature, but it seems like a feature that does more harm than good. If you really want that feature, you can just swallow all errors yourself.

[deivid-rodriguez]

It didn't go ignored, it just took a lot to discuss it because people are busy. But as I said somewhere else, it actually got some traction, see #99.