Skip to content

Conversation

desruisseaux
Copy link
Contributor

Remove the dependency to Plexus, replaced by more reliance on java.nio.

Work items

  • Replacement of Plexus includes/excludes filters by java.nio.file.PathMatcher. A benefit is the support of different syntax, at least "glob" and "regex". If no syntax is specified, default to the "glob" syntax with modifications for reproducing the behavior of Plexus filters (see below).
  • Use java.nio.file.FileVisitor for walking over files and directory trees. Consequently, symbolic links are now followed by FileVisitor instead of by maven-clean-plugin itself. An advantage is that FileVisitor is safe against infinite loops when there is cycles. Also, file attributes (whether the file is regular, a directory or a link) are queried only once per file.
  • Changes in some logging messages. The "Deleting XYZ" message is replaced by either "Deleted XYZ" if the deletion has been successful, or "Failed to delete XYZ" in case of failure (i.e., "XYZ" is not logged twice in case of failure).
  • The IOException throws by Java is no longer wrapped in another IOException, so that the callers can catch an exception of the specific sub-type if desired. The exception is also thrown earlier, before it causes another exception. The difference can be seen in the tests: a deletion fails because unauthorized, but the error that was reported to the user was not the AccessDeniedException. Instead, it was a DirectoryNotEmptyException thrown when the plugin tried to delete the directory that contains the file that the plugin failed to delete. After this commit, the exception thrown is the original AccessDeniedException.

Matcher syntax

The "glob" syntax of java.nio.file.PathMatcher seems to have slightly different rules than Plexus. In particular, the tests suggest that the ** pattern means "0 or more directories" in Maven 3 whereas the "glob" syntax of PathMatcher seems to understand ** as "1 or more directories". This commit applies the following rules:

  • If the "glob" syntax or any other syntax was explicitly specified, use the pattern verbatim. No modification applied.
  • Otherwise, modify the pattern as below:
    • Replace all occurrences of the OS-specific separator by / (the standardized separator expected by PathMatcher).
    • For every occurrence of **, generate new patterns without the ** in order to simulate the case of 0 directory.
    • Prepend glob: to the resulting pattern.

Risk

This commit is a significant changes. While the JUnir and integration tests pass, there is still a risk for some unforeseen behavioural changes. Differences compared to the previous version will be addressed as they are reported. There is also non-trivial optimizations for deciding whether to skip and entire directory (those optimizations were already present in the previous version, but behind Plexus).

Future work

If experience shows that the new version is working well, maybe we should move the Selector class and an abstract FileVisitor base class to Maven core for allowing other plugins, such as maven-compiler-plugin, to use it.

…io`.

This commit contains the following work items:

* Replacement of Plexus includes/excludes filters by `java.nio.file.PathMatcher`.
  One benefit is the support of different syntax, at least "glob" and "regex".
  If no syntax is specified, default to the "glob" syntax with modifications
  for reproducing the behavior of Plexus filters when it differs from "glob".

* Use `java.nio.file.FileVisitor` for walking over files and directory trees.
  Consequently, the following of symbolic links is now handled by `FileVisitor`
  instead of by `maven-clean-plugin` itself. An advantage is that `FileVisitor`
  is safe against infinite loops when there is cycles in the symbolic links.
  Also, file attributes (whether the file is regular, a directory or a link)
  are queried only once per file.

* Changes in some logging messages and exceptions. "Deleting XYZ" is replaced
  by either "Deleted XYZ" if the deletion has been successful, or replaced by
  "Failed to delete XYZ" in case of failure (i.e., "XYZ" is not logged twice
  in case of failure).

* The `IOException` throws by Java is no longer wrapped in another `IOException`,
  so that the callers can catch an exception of the specific sub-type if desired.
  The exception is also thrown earlier, before it causes another exception.
  The difference can be seen in the tests: a deletion fails because unauthorized,
  but the error that was reported to the user was not the `AccessDeniedException`.
  Instead, it was a `DirectoryNotEmptyException` thrown when the plugin tried to
  delete the directory that contains the file that the plugin failed to delete.
  After this commit, the exception throws is the original `AccessDeniedException`.
@desruisseaux
Copy link
Contributor Author

JUnit test failures in CleanMojoTest.testFollowLinksWithWindowsJunction, which is executed only on Windows. The failed assertion (line 262) is:

file = testDir.resolve("org-dir", "file.txt");
// (…snip…)
assertTrue(Files.exists(file));

I would need a Windows machine for debugging. But maybe it is related to the following code in the previous version, that I didn't reproduced since following symbolic links became delegated to FileVisitor:

private boolean isSymbolicLink(Path path) throws IOException {
    BasicFileAttributes attrs = Files.readAttributes(path, BasicFileAttributes.class, LinkOption.NOFOLLOW_LINKS);
    // MCLEAN-93: NTFS junctions have isDirectory() and isOther() attributes set
    return attrs.isSymbolicLink() || (attrs.isDirectory() && attrs.isOther());
}

It was used as below in the delete method of previous version:

final boolean isSymlink = isSymbolicLink(file);
Path canonical = followSymlinks ? file : getCanonicalPath(file);
if (followSymlinks || !isSymlink) {
    String prefix = !pathname.isEmpty() ? pathname + File.separatorChar : "";
    try (Stream<Path> children = Files.list(canonical)) {
        // File deleted here.
    }
}

Will attempt to reproduce that by launching a new FileVisitor when a Windows NTFS junction is detected.

@desruisseaux desruisseaux merged commit 26a0ca6 into apache:master Mar 31, 2025
6 of 8 checks passed
@github-actions github-actions bot added this to the 4.0.0-beta-3 milestone Mar 31, 2025
@desruisseaux
Copy link
Contributor Author

Sorry, pushed to the wrong repository! Will roll back now.

@gnodet
Copy link
Contributor

gnodet commented Apr 1, 2025

Future work

If experience shows that the new version is working well, maybe we should move the Selector class and an abstract FileVisitor base class to Maven core for allowing other plugins, such as maven-compiler-plugin, to use it.

The scanner is used in maven-filtering which is used in maven-resources-plugin. The fact that the Maven 4 API exposes PathMatcher while the plexus scanner uses String[] is a problematic. So yes, definitely interested in having this piece of code reusable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants