You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "AlexanderAshitkin (via GitHub)" <gi...@apache.org> on 2023/08/21 17:30:36 UTC
[GitHub] [maven-build-cache-extension] AlexanderAshitkin commented on a diff in pull request #91: [MBUILDCACHE-64] Exclusion mechanism bugfix
AlexanderAshitkin commented on code in PR #91:
URL: https://github.com/apache/maven-build-cache-extension/pull/91#discussion_r1298793252
##########
src/main/java/org/apache/maven/buildcache/checksum/MavenProjectInput.java:
##########
@@ -165,45 +184,94 @@ public MavenProjectInput(
this.repoSystem = repoSystem;
this.remoteCache = remoteCache;
Properties properties = project.getProperties();
- this.dirGlob = properties.getProperty(CACHE_INPUT_GLOB_NAME, config.getDefaultGlob());
+ this.defaultFilenameGlob = properties.getProperty(CACHE_INPUT_GLOB_NAME, config.getDefaultGlob());
this.processPlugins =
Boolean.parseBoolean(properties.getProperty(CACHE_PROCESS_PLUGINS, config.isProcessPlugins()));
this.tmpDir = System.getProperty("java.io.tmpdir");
+ this.baseDirectoryGlob = baseDirPath.toString().replace("\\", "/") + "/";
Review Comment:
For consistency, this should probably be normalized to a `<path>/**` format, or the variable should be renamed `*Path`.
##########
src/main/java/org/apache/maven/buildcache/checksum/MavenProjectInput.java:
##########
@@ -165,45 +184,94 @@ public MavenProjectInput(
this.repoSystem = repoSystem;
this.remoteCache = remoteCache;
Properties properties = project.getProperties();
- this.dirGlob = properties.getProperty(CACHE_INPUT_GLOB_NAME, config.getDefaultGlob());
+ this.defaultFilenameGlob = properties.getProperty(CACHE_INPUT_GLOB_NAME, config.getDefaultGlob());
this.processPlugins =
Boolean.parseBoolean(properties.getProperty(CACHE_PROCESS_PLUGINS, config.isProcessPlugins()));
this.tmpDir = System.getProperty("java.io.tmpdir");
+ this.baseDirectoryGlob = baseDirPath.toString().replace("\\", "/") + "/";
+
org.apache.maven.model.Build build = project.getBuild();
- filteredOutPaths = new ArrayList<>(Arrays.asList(
- normalizedPath(build.getDirectory()), // target by default
- normalizedPath(build.getOutputDirectory()),
- normalizedPath(build.getTestOutputDirectory())));
+ addToExcludedSection(
+ convertToPathMatcherFileSeperator(
+ normalizedPath(build.getDirectory()).toString())
+ + GLOB_SX_ALL_SUB_FILES,
+ false); // target by default
+ addToExcludedSection(
+ convertToPathMatcherFileSeperator(
+ normalizedPath(build.getOutputDirectory()).toString())
+ + GLOB_SX_ALL_SUB_FILES,
+ false); // target/classes by default
+ addToExcludedSection(
+ convertToPathMatcherFileSeperator(
+ normalizedPath(build.getTestOutputDirectory()).toString())
+ + GLOB_SX_ALL_SUB_FILES,
+ false); // target/test-classes by default
List<Exclude> excludes = config.getGlobalExcludePaths();
for (Exclude excludePath : excludes) {
- filteredOutPaths.add(Paths.get(excludePath.getValue()));
+ addToExcludedSection(excludePath.getValue(), true);
}
for (String propertyName : properties.stringPropertyNames()) {
if (propertyName.startsWith(CACHE_EXCLUDE_NAME)) {
String propertyValue = properties.getProperty(propertyName);
- Path path = Paths.get(propertyValue);
- filteredOutPaths.add(path);
+ addToExcludedSection(propertyValue, true);
+
if (LOGGER.isDebugEnabled()) {
LOGGER.debug(
- "Adding an excludePath from property '{}', values is '{}', path is '{}' ",
- propertyName,
- propertyValue,
- path);
+ "Adding an excludePath from property '{}', value is '{}'", propertyName, propertyValue);
}
}
}
CacheUtils.debugPrintCollection(
- LOGGER,
- filteredOutPaths,
- "List of excluded paths (checked either by fileName or by startsWith prefix)",
- "Path entry");
+ LOGGER, inputExcludePathMatcherString, "List of excluded glob patterns", "Pattern");
this.fileComparator = new PathIgnoringCaseComparator();
}
+ private String convertToPathMatcherFileSeperator(String path) {
+ return path.replace("\\", "/");
+ }
+
+ /**
+ * Add a value from the excluded section list to the directories and/or the filenames ban list.
+ * @param excludedValue a value from the exclude list
+ */
+ private void addToExcludedSection(String excludedValue, boolean addProjectBaseDir) {
+
+ String pathMatcherGlob = GLOB_PX
+ +
+ // Add the base directory to any input directly coming from user configuration
+ (addProjectBaseDir ? baseDirectoryGlob : "")
+ +
+ // If the glob start with "/", we remove it since it's already added in the added basedir glob
+ (excludedValue.startsWith("/") ? excludedValue.substring(1) : excludedValue);
+
+ // In order to skip unnecessary subtree dir walking, we use a different PathMatcher list for "directories" or
+ // "files + directories"
+ inputExcludePathMatchers.add(new TreeWalkerPathMatcher(pathMatcherGlob, false));
+
+ // Globs ending with "**" should end any sub-directory inspection
+ if (pathMatcherGlob.endsWith("**")) {
Review Comment:
This is a very thin ice. Glob syntax is rich. For example, it could be `[*][*]` or `/my/fav/dir/{**}` or `/my/*/dir/**`. It is a fragile approach to interpreting the globs based on the content. `excludedValue` could be anything, and the logic might not be applied properly. That's why the original logic operated with normalized paths.
##########
src/main/java/org/apache/maven/buildcache/checksum/MavenProjectInput.java:
##########
@@ -165,45 +184,94 @@ public MavenProjectInput(
this.repoSystem = repoSystem;
this.remoteCache = remoteCache;
Properties properties = project.getProperties();
- this.dirGlob = properties.getProperty(CACHE_INPUT_GLOB_NAME, config.getDefaultGlob());
+ this.defaultFilenameGlob = properties.getProperty(CACHE_INPUT_GLOB_NAME, config.getDefaultGlob());
this.processPlugins =
Boolean.parseBoolean(properties.getProperty(CACHE_PROCESS_PLUGINS, config.isProcessPlugins()));
this.tmpDir = System.getProperty("java.io.tmpdir");
+ this.baseDirectoryGlob = baseDirPath.toString().replace("\\", "/") + "/";
+
org.apache.maven.model.Build build = project.getBuild();
- filteredOutPaths = new ArrayList<>(Arrays.asList(
- normalizedPath(build.getDirectory()), // target by default
- normalizedPath(build.getOutputDirectory()),
- normalizedPath(build.getTestOutputDirectory())));
+ addToExcludedSection(
+ convertToPathMatcherFileSeperator(
+ normalizedPath(build.getDirectory()).toString())
+ + GLOB_SX_ALL_SUB_FILES,
+ false); // target by default
+ addToExcludedSection(
+ convertToPathMatcherFileSeperator(
+ normalizedPath(build.getOutputDirectory()).toString())
+ + GLOB_SX_ALL_SUB_FILES,
+ false); // target/classes by default
+ addToExcludedSection(
+ convertToPathMatcherFileSeperator(
+ normalizedPath(build.getTestOutputDirectory()).toString())
+ + GLOB_SX_ALL_SUB_FILES,
+ false); // target/test-classes by default
List<Exclude> excludes = config.getGlobalExcludePaths();
for (Exclude excludePath : excludes) {
- filteredOutPaths.add(Paths.get(excludePath.getValue()));
+ addToExcludedSection(excludePath.getValue(), true);
}
for (String propertyName : properties.stringPropertyNames()) {
if (propertyName.startsWith(CACHE_EXCLUDE_NAME)) {
String propertyValue = properties.getProperty(propertyName);
- Path path = Paths.get(propertyValue);
- filteredOutPaths.add(path);
+ addToExcludedSection(propertyValue, true);
+
if (LOGGER.isDebugEnabled()) {
LOGGER.debug(
- "Adding an excludePath from property '{}', values is '{}', path is '{}' ",
- propertyName,
- propertyValue,
- path);
+ "Adding an excludePath from property '{}', value is '{}'", propertyName, propertyValue);
}
}
}
CacheUtils.debugPrintCollection(
- LOGGER,
- filteredOutPaths,
- "List of excluded paths (checked either by fileName or by startsWith prefix)",
- "Path entry");
+ LOGGER, inputExcludePathMatcherString, "List of excluded glob patterns", "Pattern");
this.fileComparator = new PathIgnoringCaseComparator();
}
+ private String convertToPathMatcherFileSeperator(String path) {
+ return path.replace("\\", "/");
+ }
+
+ /**
+ * Add a value from the excluded section list to the directories and/or the filenames ban list.
+ * @param excludedValue a value from the exclude list
+ */
+ private void addToExcludedSection(String excludedValue, boolean addProjectBaseDir) {
+
+ String pathMatcherGlob = GLOB_PX
+ +
+ // Add the base directory to any input directly coming from user configuration
+ (addProjectBaseDir ? baseDirectoryGlob : "")
+ +
+ // If the glob start with "/", we remove it since it's already added in the added basedir glob
+ (excludedValue.startsWith("/") ? excludedValue.substring(1) : excludedValue);
Review Comment:
Previously, the logic was `Path` API based. And if a value could not be converted to a path, it threw an exception immediately. Some erroneous values (with, say, illegal characters) could be added now because the input value doesn't pass any validation and is taken directly as is.
##########
src/main/java/org/apache/maven/buildcache/checksum/MavenProjectInput.java:
##########
@@ -127,6 +121,14 @@ public class MavenProjectInput {
* Flag to control if we should check values from plugin configs as file system objects
*/
private static final String CACHE_PROCESS_PLUGINS = "maven.build.cache.processPlugins";
+ /**
+ * Glob prefix for path matchers
+ */
+ private static final String GLOB_PX = "glob:";
+ /**
+ * Glob suffix meaning "all files in this directory or any sub-directory"
+ */
+ private static final String GLOB_SX_ALL_SUB_FILES = "/**";
Review Comment:
I would expect `/*` for all sub-files. `GLOB_SX_FULL_SUB_TREE` could be a more accurate naming for a full depth subtree.
##########
src/main/java/org/apache/maven/buildcache/checksum/MavenProjectInput.java:
##########
@@ -633,13 +714,7 @@ private static boolean isReadable(Path entry) throws IOException {
}
private boolean isFilteredOutSubpath(Path path) {
- Path normalized = path.normalize();
- for (Path filteredOutDir : filteredOutPaths) {
- if (normalized.startsWith(filteredOutDir)) {
- return true;
- }
- }
- return false;
+ return inputExcludeDirectoryPathMatchers.stream().anyMatch(matcher -> matcher.stopTreeWalking(path));
Review Comment:
The logic for skipping dirs should be paths based. Globs should be used as globs without any content assumptions. To get the benefits of both approaches, creating a wrapper class for the globs might be better. And if the glob is path-based (eg derived by adding `/**`, some optimizations could be applied). Which will keep consistent values of the original literal, derived path, and the derived glob.
##########
src/main/java/org/apache/maven/buildcache/checksum/TreeWalkerPathMatcher.java:
##########
@@ -0,0 +1,53 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.buildcache.checksum;
+
+import java.nio.file.FileSystems;
+import java.nio.file.Path;
+import java.nio.file.PathMatcher;
+
+/**
+ * A path matcher with some extra info
+ */
+public class TreeWalkerPathMatcher implements PathMatcher {
+
+ /**
+ * True if the matching should stop exploring the directory tree further away
+ */
+ private final boolean matchingSkipSubtree;
+
+ /**
+ * Wrapped regular path matcher
+ */
+ private final PathMatcher pathMatcher;
+
+ public TreeWalkerPathMatcher(String pathMatcherGlob, boolean matchingSkipSubtree) {
+ pathMatcher = FileSystems.getDefault().getPathMatcher(pathMatcherGlob);
+ this.matchingSkipSubtree = matchingSkipSubtree;
+ }
+
+ @Override
+ public boolean matches(Path path) {
Review Comment:
We may need a wrapper type here to ensure consistency in the path matching. The second, less preferable option is to normalize paths within the matcher. But in the latter case, the normalized paths cannot be reused in the other parts of the code.
##########
src/main/java/org/apache/maven/buildcache/checksum/TreeWalkerPathMatcher.java:
##########
@@ -0,0 +1,53 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.maven.buildcache.checksum;
+
+import java.nio.file.FileSystems;
+import java.nio.file.Path;
+import java.nio.file.PathMatcher;
+
+/**
+ * A path matcher with some extra info
+ */
+public class TreeWalkerPathMatcher implements PathMatcher {
+
+ /**
+ * True if the matching should stop exploring the directory tree further away
+ */
+ private final boolean matchingSkipSubtree;
+
+ /**
+ * Wrapped regular path matcher
+ */
+ private final PathMatcher pathMatcher;
Review Comment:
Could it lead to OS-specific errors? For example, if we replace the slashes, could that break the path matching on Windows? Ideally, we need to encapsulate `Path`s and work on the normalized paths only to ensure portability.
##########
src/test/java/org/apache/maven/buildcache/its/IncludeExcludeTest.java:
##########
@@ -0,0 +1,107 @@
+/*
Review Comment:
Thank you for implementing this test. It is long past due to implement 👍
##########
src/main/java/org/apache/maven/buildcache/checksum/MavenProjectInput.java:
##########
@@ -165,45 +184,94 @@ public MavenProjectInput(
this.repoSystem = repoSystem;
this.remoteCache = remoteCache;
Properties properties = project.getProperties();
- this.dirGlob = properties.getProperty(CACHE_INPUT_GLOB_NAME, config.getDefaultGlob());
+ this.defaultFilenameGlob = properties.getProperty(CACHE_INPUT_GLOB_NAME, config.getDefaultGlob());
this.processPlugins =
Boolean.parseBoolean(properties.getProperty(CACHE_PROCESS_PLUGINS, config.isProcessPlugins()));
this.tmpDir = System.getProperty("java.io.tmpdir");
+ this.baseDirectoryGlob = baseDirPath.toString().replace("\\", "/") + "/";
+
org.apache.maven.model.Build build = project.getBuild();
- filteredOutPaths = new ArrayList<>(Arrays.asList(
- normalizedPath(build.getDirectory()), // target by default
- normalizedPath(build.getOutputDirectory()),
- normalizedPath(build.getTestOutputDirectory())));
+ addToExcludedSection(
Review Comment:
There is room for a small optimization here - not adding sub-dirs of the `target` will reduce the length of the filter.
##########
src/main/java/org/apache/maven/buildcache/checksum/MavenProjectInput.java:
##########
@@ -526,6 +603,10 @@ public FileVisitResult visitFileFailed(Path path, IOException exc) throws IOExce
});
}
+ private boolean entryMustBeSkipped(Path path) {
+ return inputExcludePathMatchers.stream().anyMatch(pathMatcher -> pathMatcher.matches(path));
Review Comment:
neat picking: use method reference instead of the lambda - `PathMatcher::matches`
##########
src/main/java/org/apache/maven/buildcache/checksum/MavenProjectInput.java:
##########
@@ -165,45 +184,94 @@ public MavenProjectInput(
this.repoSystem = repoSystem;
this.remoteCache = remoteCache;
Properties properties = project.getProperties();
- this.dirGlob = properties.getProperty(CACHE_INPUT_GLOB_NAME, config.getDefaultGlob());
+ this.defaultFilenameGlob = properties.getProperty(CACHE_INPUT_GLOB_NAME, config.getDefaultGlob());
Review Comment:
The glob is not limited to filenames, and the exact format is not guaranteed, etc. There is no base to incorporate the content assumption in the variable name. Better naming would be `projectGlob`, which properly reflects the intention - a project-specific glob, overriding default if specified.
##########
src/test/java/org/apache/maven/buildcache/its/IncludeExcludeTest.java:
##########
@@ -0,0 +1,107 @@
+/*
Review Comment:
Thank you for implementing this test. It is long past due to implement 👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org