[ruby-core:109701] [Ruby master Bug#18784] `FileUtils.rm_f` and `FileUtils.rm_rf` should not mask exceptions
From:
"mame (Yusuke Endoh)" <noreply@...>
Date:
2022-08-26 04:08:18 UTC
List:
ruby-core #109701
Issue #18784 has been updated by mame (Yusuke Endoh).
Unfortunately, this change affected mkmf on Appveyor (Windows).
https://ci.appveyor.com/project/ruby/ruby/builds/44579924/job/n81xmb2mqs6no7dm#L23792
```
2) Error:
TestMkmfTryConstant#test_simple:
Errno::EACCES: Permission denied @ apply2files - conftest.exe
C:/projects/ruby/lib/fileutils.rb:2300:in `unlink'
C:/projects/ruby/lib/fileutils.rb:2300:in `block in remove_file'
C:/projects/ruby/lib/fileutils.rb:2308:in `platform_support'
C:/projects/ruby/lib/fileutils.rb:2299:in `remove_file'
C:/projects/ruby/lib/fileutils.rb:1449:in `remove_file'
C:/projects/ruby/lib/fileutils.rb:1189:in `block in rm'
C:/projects/ruby/lib/fileutils.rb:1188:in `each'
C:/projects/ruby/lib/fileutils.rb:1188:in `rm'
C:/projects/ruby/lib/fileutils.rb:1211:in `rm_f'
C:/projects/ruby/lib/mkmf.rb:260:in `rm_f'
C:/projects/ruby/lib/mkmf.rb:768:in `ensure in try_constant'
C:/projects/ruby/lib/mkmf.rb:768:in `try_constant'
C:/projects/ruby/test/mkmf/test_constant.rb:6:in `block in test_simple'
C:/projects/ruby/test/mkmf/base.rb:132:in `instance_eval'
C:/projects/ruby/test/mkmf/base.rb:132:in `mkmf'
C:/projects/ruby/test/mkmf/test_constant.rb:6:in `test_simple'
```
mkmf uses FileUtils.rm_f to delete conftest.exe, which is a temporary executable. But this seems to fail occasionally.
According to @nobu, mkmf tries to delete conftest.exe after the child process terminated. However, it may fail to delete an executable even immediately after execution ends. (Or, another process might lock the file somehow.)
This issue cannot reproduce on my Windows machine, so I cannot figure out the cause of the problem.
Note that @k0kubun has disabled the tests in mkmf on Appveyor, so I think this failure of mkmf will decrease.
I am wondering if I should revert the change of FileUtils. I know that reverting will not solve the problem of mkmf. But this problem is harder to fix than I expected. I concern that this change will make users to just ignore exceptions of FileUtils.rm_rf, like `begin; FileUtils.rm_rf(...); rescue SystemCallError; end`. This is not what I wanted.
I will bring this reconsideration to the next dev meeting.
----------------------------------------
Bug #18784: `FileUtils.rm_f` and `FileUtils.rm_rf` should not mask exceptions
https://bugs.ruby-lang.org/issues/18784#change-98927
* Author: deivid (David Rodr鱈guez)
* Status: Open
* Priority: Normal
* ruby -v: ruby 3.1.2p20 (2022-04-12 revision 4491bb740a) [arm64-darwin21]
* Backport: 2.7: UNKNOWN, 3.0: UNKNOWN, 3.1: UNKNOWN
----------------------------------------
In recent times, I've been having issues with these methods because they don't let you know when some issue happened while trying to remove the given folders/files.
IMO most users expect all pre-existing folders/files that are passed to these methods to be actually removed by the methods. Instead, when this happens, errors are silently swallowed and normally the result is that you will get some other issue further down the road, making the problem hard to debug.
The current workaround I'm using is to double check whether the files still exist after the method, and raise a custom error if they do, but I still can't see the original problem, so issues are similarly hard to debug.
This is also a deviation from how `rm -rf` and `rm -f` work, since these tools finish with a failure exit code when they fail to remove the given files. Given that `fileutils` is intended to mimic shell functionality, I think this is just a bug.
I think the intention of the `force` flag here is to:
* Don't prompt for confirmation.
* Ignore given arguments that are not files that already exist.
But any issue other than that should not be swallowed, and in general I think the method should succeed if and only if the given list of file names does not exist after the methods are done.
I think this is in line with the following note I get when I run `man rm`, but also suggests that this is not the standard behavior of "historical implementations"
> 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.
I implemented this at https://github.com/ruby/fileutils/pull/58, but treating this as a bug. I can also implement a more conservative for approach for users that might be using `FileUtils.rm_rf` or `FileUtils.rm_f` but don't really care if the files are removed or not.
Alternative proposals would be `FileUtils.rm_rf(force: strict)`, or `FileUtils.strict_rm_rf`, but to be honest, if this is considered a breaking change, I would ship it as a new major version, and let users update their code to swallow errors themselves if they need to.
Happy to hear any feedback!
--
https://bugs.ruby-lang.org/
Unsubscribe: <mailto:ruby-core-request@ruby-lang.org?subject=unsubscribe>
<http://lists.ruby-lang.org/cgi-bin/mailman/options/ruby-core>