Add way to delete read-only files

[raydac]

New feature, improvement proposal

Some plugins can create read only files which deletion throws java.nio.file.AccessDeniedException in maven-clean-plugin, it would be useful to provide a way to change file attributes before delete to fix such exception

[mira-silhavy]

We have the same problem, updating to 3.4.1 from 3.4.0 and clean plugin is failing to delete files and throw AccessDeniedException.
This should be a bug and not an enhancement @raydac

Caused by: java.nio.file.AccessDeniedException: C:\_Work_\git\test\main\domain\target\generated-sources\xjc\com\test\TestEnum.java
    at sun.nio.fs.WindowsException.translateToIOException (WindowsException.java:89)
    at sun.nio.fs.WindowsException.rethrowAsIOException (WindowsException.java:103)
    at sun.nio.fs.WindowsException.rethrowAsIOException (WindowsException.java:108)
    at sun.nio.fs.WindowsFileSystemProvider.implDelete (WindowsFileSystemProvider.java:275)
    at sun.nio.fs.AbstractFileSystemProvider.delete (AbstractFileSystemProvider.java:105)
    at java.nio.file.Files.delete (Files.java:1152)
    at org.apache.maven.plugins.clean.Cleaner.delete (Cleaner.java:316)
    at org.apache.maven.plugins.clean.Cleaner.delete (Cleaner.java:276)
    at org.apache.maven.plugins.clean.Cleaner.delete (Cleaner.java:250)
    at org.apache.maven.plugins.clean.Cleaner.delete (Cleaner.java:222)
    at org.apache.maven.plugins.clean.Cleaner.delete (Cleaner.java:222)
    at org.apache.maven.plugins.clean.Cleaner.delete (Cleaner.java:222)
    at org.apache.maven.plugins.clean.Cleaner.delete (Cleaner.java:222)
    at org.apache.maven.plugins.clean.Cleaner.delete (Cleaner.java:222)
    at org.apache.maven.plugins.clean.Cleaner.delete (Cleaner.java:222)
    at org.apache.maven.plugins.clean.Cleaner.delete (Cleaner.java:222)
    at org.apache.maven.plugins.clean.Cleaner.delete (Cleaner.java:222)
    at org.apache.maven.plugins.clean.Cleaner.delete (Cleaner.java:222)
    at org.apache.maven.plugins.clean.Cleaner.delete (Cleaner.java:128)
    at org.apache.maven.plugins.clean.CleanMojo.execute (CleanMojo.java:257)

[slawekjaranowski]

PR are welcome ....

We also should to think:

• should this be the default option?
• what with errors during attribute changing

[anthonyvdotbe]

As mentioned above, this is a regression from 3.4.0, not an enhancement.
The root cause is that, at least on Windows, File::delete deletes read-only files successfully, whereas Files::delete does not.

The regression was introduced by the work on migrating to java.nio.file APIs:

• e8fea70
• 091570b
• cb2127f
• bf21c55

@slawekjaranowski I'd argue that the burden should not be on the "victims" to provide a PR. Instead, I propose to revert the problematic commits and release version 3.4.2

[desruisseaux]

I think that we need to keep the migration to Path. Other work such as #243 are built on top of those commits. This work replaces Plexus by java.nio.file.PathMatcher and java.nio.file.FileVisitor, which allows for example to delegate the handling of symbolic links to Java. It is also part of a larger effort to reduce inconsistent duplications among Maven plugins (e.g., maven-clean-plugin and maven-compiler-plugin duplicate codes for iterating over files, but with slight behavioural differences that are probably unintentional).

Instead, I propose to add a force configuration option similar to rm -f in Unix. I suggest that the default value should be false for safety and for consistency with the standard usage in Unix commands. If agreed, I can volunteer for adding this option on top of #243.

[anthonyvdotbe]

Just to be clear: I'm all in favor of migrating to Path. I'm just arguing that this issue is a severe regression and that we should revert the problematic commits and release version 3.4.2 ASAP. Parallel to that, a new PR can be created (for which you can just revert the reverted commits again and add tests to it to prevent any regressions like this one). #243 is not merged yet, so it could just be rebased on the new PR.

Instead, I propose to add a force configuration option similar to rm -f in Unix. I suggest that the default value should be false for safety and for consistency with the standard usage in Unix commands. If agreed, I can volunteer for adding this option on top of #243.

The new behavior is a regression that breaks long-standing behavior. So I believe the priority should be on fixing the regression.
Besides that, I fail to see why anyone would want to use such a configuration option with the value false?
Anyway, if such an option were to be introduced, its default value should match the way it has always behaved, so true.

[desruisseaux]

We can see the 3.4.1 behaviour as a regression, or the 3.4.0 behaviour as a bug. Both views are defensible. A compromise could be to default to true for now with a warning saying that the default would change to false in a future version (that warning would not be emitted if the user explicitly set that option).

A reason for safety is that there is always a risk that the directory to delete is accidentally set to the wrong directory. Or maybe there is a symbolic link pointing outside the target directory (e.g. users created it temporarily for a test, then forgot about it) and the plugin is following symbolic links. If a file is marked as read-only, it is probably of particular importance for the user, so being conservative is a common choice.

[anthonyvdotbe]

This issue was filed just 10 days after the release of 3.4.1, while I doubt there have ever been complaints about the deletion of read-only files in the decades before 3.4.1

Anyway, to each their own. I'll just revert to 3.4.0 and leave this discussion to the powers that be.

[desruisseaux]

Started a branch for the addition of a force configuration parameter. Still work in progress at the time of writing this comment.

[desruisseaux]

Completed a first attempt. If any volunteer would like to build the branch locally and test, feedbacks would be welcome as there is potential for corner cases not correctly handled.

[slawekjaranowski]

@desruisseaux I will try port a force option to 3.x branch

[desruisseaux]

@anthonyvdotbe : apparently, reverting the commits may not work anyway. Oracle seems to also consider that File.delete deleting read-only files is a bug. See https://bugs.openjdk.org/browse/JDK-8355954

[anthonyvdotbe]

Context is everything. Oracle's assessment of the situation and their chosen path forward makes perfect sense to me. At the same time, introducing a force parameter with a default value of false to deal with this regression makes no sense to me.