Skip to content
This repository has been archived by the owner on Dec 5, 2023. It is now read-only.

Commit

Permalink
Merge pull request #26452 Fix order dependency in excluded group
Browse files Browse the repository at this point in the history
Signed-off-by: K Ke \ [email protected]

### Context
HashSet default iteration order changed between Java 7 and 8, breaking lots of unit tests. Our NonDex plugin https://github.com/TestingResearchIllinois/NonDex permutes these orders just as between Java 7 and 8, and it triggers the following error when testing the Gradle building process of a plugin repeatedly:
```
org.gradle.api.InvalidUserCodeException: Cannot change default excludes during the build. They were changed from [foo, bar, xyz] to [foo, bar, xyz]
```

### Reason
`PatternSpecFactory` is making sure that excluded group is not changed during the build process. In line 91 of `PatternSpecFactory.java`, `DirectoryScanner.getDefaultExcludes()` is called to get the list of excluded groups in String format. However, it can be see that Ant simply casted their `HashSet` of `defaultExcludes` to `Array` in their implementation of  `getDefaultExcludes()` (See https://github.com/apache/ant/blob/72d6a274733d7c5460efc9ae53d645246184dabe/src/main/org/apache/tools/ant/DirectoryScanner.java#L571). However, Oracle document specifies that `HashSet` does not ensure iteration order (https://docs.oracle.com/javase/8/docs/api/java/util/HashSet.html), so the returned Array of `getDefaultExcludes()` does not ensure order. Therefore, if we run a "assert Gradle build success for plugin" unit test repeatedly, we can trigger `invalidChangeOfExcludes()` (line 102 in `PatternSpecFactory.java`) as the array of excluded group strings may have order permutation.

### Significance of Fixing
The HashMap and HashSet default iteration order did change between Java 7 and 8, and may change again in the future, even if it appears deterministic now. Moreover, the list of `excluded files` should not rely on any order by nature.

### Suggested Fix
Convert the `Array` of excluded groups to `HashSet` before comparison. This is efficient with O(n+n) runtime - it will not increase overhead as much as `sort()` would.

### Contributor Checklist
- [X] [Review Contribution Guidelines](https://github.com/gradle/gradle/blob/master/CONTRIBUTING.md)
- [X] Make sure that all commits are [signed off](https://git-scm.com/docs/git-commit#Documentation/git-commit.txt---signoff) to indicate that you agree to the terms of [Developer Certificate of Origin](https://developercertificate.org/).
- [X] Make sure all contributed code can be distributed under the terms of the [Apache License 2.0](https://github.com/gradle/gradle/blob/master/LICENSE), e.g. the code was written by yourself or the original code is licensed under [a license compatible to Apache License 2.0](https://apache.org/legal/resolved.html).
- [X] Check ["Allow edit from maintainers" option](https://help.github.com/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork/) in pull request so that additional changes can be pushed by Gradle team
- [ ] Provide integration tests (under `<subproject>/src/integTest`) to verify changes from a user perspective
- [ ] Provide unit tests (under `<subproject>/src/test`) to verify logic
- [ ] Update User Guide, DSL Reference, and Javadoc for public-facing changes
- [X] Ensure that tests pass sanity check: `./gradlew sanityCheck`
- [X] Ensure that tests pass locally: `./gradlew <changed-subproject>:quickTest`

### Reviewing cheatsheet

Before merging the PR, comments starting with
- ❌ ❓**must** be fixed
- 🤔 💅 **should** be fixed
- 💭 **may** be fixed
- 🎉 celebrate happy things

Co-authored-by: Kaiyao Ke <[email protected]>
  • Loading branch information
bot-gradle and kaiyaok2 committed Nov 27, 2023
2 parents a593e13 + 693b1f1 commit ad1c6ad
Showing 1 changed file with 14 additions and 12 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,10 @@
import java.util.Collection;
import java.util.Comparator;
import java.util.EnumMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;

/**
* The basic implementation for converting {@link PatternSet}s to {@link Spec}s.
Expand All @@ -46,7 +48,7 @@
@ServiceScope(Scope.Global.class)
public class PatternSpecFactory implements FileSystemDefaultExcludesListener {
public static final PatternSpecFactory INSTANCE = new PatternSpecFactory();
private String[] previousDefaultExcludes;
private Set<String> previousDefaultExcludes = new HashSet<String>();
private final Map<CaseSensitivity, Spec<FileTreeElement>> defaultExcludeSpecCache = new EnumMap<>(CaseSensitivity.class);

@Override
Expand Down Expand Up @@ -88,7 +90,7 @@ public Spec<FileTreeElement> createExcludeSpec(PatternSet patternSet) {
}

private synchronized Spec<FileTreeElement> getDefaultExcludeSpec(CaseSensitivity caseSensitivity) {
String[] defaultExcludes = DirectoryScanner.getDefaultExcludes();
Set<String> defaultExcludes = new HashSet<String>(Arrays.asList(DirectoryScanner.getDefaultExcludes()));
if (defaultExcludeSpecCache.isEmpty()) {
updateDefaultExcludeSpecCache(defaultExcludes);
} else if (invalidChangeOfExcludes(defaultExcludes)) {
Expand All @@ -98,29 +100,29 @@ private synchronized Spec<FileTreeElement> getDefaultExcludeSpec(CaseSensitivity
return defaultExcludeSpecCache.get(caseSensitivity);
}

private boolean invalidChangeOfExcludes(String[] defaultExcludes) {
return !Arrays.equals(previousDefaultExcludes, defaultExcludes);
private boolean invalidChangeOfExcludes(Set<String> defaultExcludes) {
return !previousDefaultExcludes.equals(defaultExcludes);
}

private void failOnChangedDefaultExcludes(String[] excludesFromSettings, String[] newDefaultExcludes) {
List<String> sortedExcludesFromSettings = Arrays.asList(excludesFromSettings);
private void failOnChangedDefaultExcludes(Set<String> excludesFromSettings, Set<String> newDefaultExcludes) {
List<String> sortedExcludesFromSettings = new ArrayList<String>(excludesFromSettings);
sortedExcludesFromSettings.sort(Comparator.naturalOrder());
List<String> sortedNewExcludes = Arrays.asList(newDefaultExcludes);
List<String> sortedNewExcludes = new ArrayList<String>(newDefaultExcludes);
sortedNewExcludes.sort(Comparator.naturalOrder());
throw new InvalidUserCodeException(String.format("Cannot change default excludes during the build. They were changed from %s to %s. Configure default excludes in the settings script instead.", sortedExcludesFromSettings, sortedNewExcludes));
}

public synchronized void setDefaultExcludesFromSettings(String[] excludesFromSettings) {
if (!Arrays.equals(previousDefaultExcludes, excludesFromSettings)) {
updateDefaultExcludeSpecCache(excludesFromSettings);
Set<String> excludesFromSettingsSet = new HashSet<String>(Arrays.asList(excludesFromSettings));
if (!previousDefaultExcludes.equals(excludesFromSettingsSet)) {
updateDefaultExcludeSpecCache(excludesFromSettingsSet);
}
}

private void updateDefaultExcludeSpecCache(String[] defaultExcludes) {
private void updateDefaultExcludeSpecCache(Set<String> defaultExcludes) {
previousDefaultExcludes = defaultExcludes;
List<String> patterns = Arrays.asList(defaultExcludes);
for (CaseSensitivity caseSensitivity : CaseSensitivity.values()) {
defaultExcludeSpecCache.put(caseSensitivity, createSpec(patterns, false, caseSensitivity.isCaseSensitive()));
defaultExcludeSpecCache.put(caseSensitivity, createSpec(defaultExcludes, false, caseSensitivity.isCaseSensitive()));
}
}

Expand Down

0 comments on commit ad1c6ad

Please sign in to comment.