Merge pull request #25261 Test for encoding problems and fix a few on…

… Linux and macOS

The overarching goal here is to shake out more encoding problems throughout Gradle by running all our integration tests in a way that the tested code has to deal with non-ASCII characters in file paths. This PR takes a step towards that goal by forcing all our non-Windows integration tests to use such a path.

To keep the scope manageable, this PR does not force non-ASCII paths for Windows. That needs to be enabled in a followup PR where we can deal with Windows-specific encoding problems.

The idea is similar to how we add a space to the `build/tmp/test files` directory's name where all the test output is typically located; this time we replace the `s` in `test files` with an `ŝ` (see [U+015D](https://www.compart.com/en/unicode/U+015D)). Importantly this character is not part of ASCII nor any ISO-8859-X codepage, and cannot be represented by a single byte. (See [Wikipedia](https://en.wikipedia.org/wiki/ŝ)).

There is also an escape hatch for tests that for some reason can't support Unicode paths; these need to be tagged with `@DoesNotSupportNonAsciiPaths`. We have such offenders today:

  - Checkstyle fails because of this bug: checkstyle/checkstyle#13012
  - Java 6 wrapper tests fail because Java 6 barfs on non-ASCII characters in the path

Most of the problems that had to be fixed for Unixes come from the fact that `URI.toString()` does not encode non-ASCII characters, and some tools can't parse string representations of URIs with non-ASCII characters in them.

So some of the `URI`s are now converted to strings using `toASCIIString()` instead. This is the canonical form of a URI, and is the intended way to go when the string form is passed to places where we can't ensure that it will be read with the right encoding (see https://www.w3.org/Addressing/URL/3_URI_Choices.html)

There are followups:
- #25316
- #25322

This PR is a followup to the daemon encoding fix in:
- #25319

Co-authored-by: Lóránt Pintér <lorant@gradle.com>

.teamcity/src/main/kotlin/common/extensions.kt

@@ -103,7 +103,7 @@ fun BuildType.applyDefaultSettings(os: Os = Os.LINUX, arch: Arch = Arch.AMD64, b
         *.psoutput => $hiddenArtifactDestination
         build/*.threaddump => $hiddenArtifactDestination
         build/report-* => $hiddenArtifactDestination
-        build/tmp/test files/** => $hiddenArtifactDestination/test-files
+        build/tmp/teŝt files/** => $hiddenArtifactDestination/teŝt-files
         build/errorLogs/** => $hiddenArtifactDestination/errorLogs
         subprojects/internal-build-reports/build/reports/incubation/all-incubating.html => incubation-reports
         build/reports/dependency-verification/** => dependency-verification-reports

build-logic-commons/gradle-plugin/src/main/kotlin/gradlebuild/testcleanup/TestFilesCleanupService.kt

@@ -191,7 +191,7 @@ abstract class TestFilesCleanupService @Inject constructor(
      * Returns non-empty directories: the mapping of directory to at most 4 leftover files' relative path in the directory.
      */
     private
-    fun TestFilesCleanupProjectState.tmpTestFiles(): LeftoverFiles = projectBuildDir.get().asFile.resolve("tmp/test files")
+    fun TestFilesCleanupProjectState.tmpTestFiles(): LeftoverFiles = projectBuildDir.get().asFile.resolve("tmp/teŝt files")
         .listFiles()
         ?.associateWith { dir ->
             val dirPath = dir.toPath()

build-logic/buildquality/src/test/kotlin/gradlebuild/testcleanup/TestFilesCleanupServiceTest.kt

@@ -105,7 +105,7 @@ class TestFilesCleanupServiceTest {
                     this.ignoreFailures = ignoreFailures
                     if (hasLeftover) {
                         doFirst {
-                            project.layout.buildDirectory.file("tmp/test files/leftover/leftover").get().asFile.apply {
+                            project.layout.buildDirectory.file("tmp/teŝt files/leftover/leftover").get().asFile.apply {
                                 parentFile.mkdirs()
                                 createNewFile()
                             }
@@ -133,7 +133,7 @@ class TestFilesCleanupServiceTest {
                     project.touchInBuildDir( "reports/report.html")
 
                     if (project.name == "failed-report-with-leftover") {
-                        project.touchInBuildDir("tmp/test files/leftover/leftover")
+                        project.touchInBuildDir("tmp/teŝt files/leftover/leftover")
                         throw IllegalStateException()
                     }
                 }
@@ -186,9 +186,9 @@ class TestFilesCleanupServiceTest {
 
         assertEquals(1, StringUtils.countMatches(result.output, "Found non-empty test files dir"))
         assertEquals(1, StringUtils.countMatches(result.output, "Failed to stop service 'testFilesCleanupBuildService'"))
-        result.output.assertContains("successful-test-with-leftover/build/tmp/test files/leftover")
+        result.output.assertContains("successful-test-with-leftover/build/tmp/teŝt files/leftover")
 
-        assertLeftoverFilesCleanedUpEventually("successful-test-with-leftover/build/tmp/test files")
+        assertLeftoverFilesCleanedUpEventually("successful-test-with-leftover/build/tmp/teŝt files")
     }
 
     @Test
@@ -215,8 +215,8 @@ class TestFilesCleanupServiceTest {
         // leftover files failed tests are reported but not counted as an exception, but cleaned up eventually
         assertEquals(1, StringUtils.countMatches(result.output, "Found non-empty test files dir"))
         assertEquals(1, StringUtils.countMatches(result.output, "Failed to stop service 'testFilesCleanupBuildService'"))
-        result.output.assertContains("failed-report-with-leftover/build/tmp/test files/leftover")
-        result.output.assertContains("failed-test-with-leftover/build/tmp/test files/leftover")
+        result.output.assertContains("failed-report-with-leftover/build/tmp/teŝt files/leftover")
+        result.output.assertContains("failed-test-with-leftover/build/tmp/teŝt files/leftover")
 
         assertArchivedFilesSeen(
             "report-failed-test-with-leftover-test.zip",
@@ -225,9 +225,9 @@ class TestFilesCleanupServiceTest {
             "report-successful-report-reports.zip"
         )
         assertLeftoverFilesCleanedUpEventually(
-            "failed-report-with-leftover/build/tmp/test files",
-            "successful-report/build/tmp/test files",
-            "failed-test-with-leftover/build/tmp/test files"
+            "failed-report-with-leftover/build/tmp/teŝt files",
+            "successful-report/build/tmp/teŝt files",
+            "failed-test-with-leftover/build/tmp/teŝt files"
         )
     }
 
@@ -237,14 +237,14 @@ class TestFilesCleanupServiceTest {
 
         // leftover files failed tests are reported but not counted as an exception, but cleaned up eventually
         assertEquals(1, StringUtils.countMatches(result.output, "Leftover files"))
-        result.output.assertContains("flaky-test-with-leftover/build/tmp/test files/leftover")
+        result.output.assertContains("flaky-test-with-leftover/build/tmp/teŝt files/leftover")
 
         assertArchivedFilesSeen(
             "report-flaky-test-with-leftover-test.zip",
             "report-flaky-test-with-leftover-leftover.zip"
         )
 
-        assertLeftoverFilesCleanedUpEventually("flaky-test-with-leftover/build/tmp/test files")
+        assertLeftoverFilesCleanedUpEventually("flaky-test-with-leftover/build/tmp/teŝt files")
     }
 
     private

build-logic/cleanup/src/test/groovy/gradlebuild/cleanup/services/LeakingProcessKillPatternTest.groovy

@@ -21,7 +21,7 @@ import spock.lang.Specification
 class LeakingProcessKillPatternTest extends Specification {
 
     def "matches worker process started in test on Windows"() {
-        def line = '"C:\\Program Files\\Java\\jdk1.7/bin/java.exe"    -Dorg.gradle.daemon.idletimeout=120000 -Dorg.gradle.daemon.registry.base=C:\\some\\agent\\workspace\\build\\daemon -Dorg.gradle.native.dir=C:\\some\\agent\\workspace\\intTestHomeDir\\worker-1\\native -Dorg.gradle.deprecation.trace=true -Djava.io.tmpdir=C:\\some\\agent\\workspace\\subprojects\\osgi\\build\\tmp -Dfile.encoding=windows-1252 -Dorg.gradle.classloaderscope.strict=true -ea -ea "-Dorg.gradle.appname=gradle" -classpath "C:\\some\\agent\\workspace\\subprojects\\osgi\\build\\integ test\\bin\\..\\lib\\gradle-launcher-4.5.jar" org.gradle.launcher.GradleMain --init-script "C:\\some\\agent\\workspace\\subprojects\\osgi\\build\\tmp\\test files\\OsgiPluginIntegrationSpec\\can_merge_manifests...archives_\\uz4kt\\reproducible-archives-init.gradle" --no-daemon --stacktrace --gradle-user-home C:\\some\\agent\\workspace\\intTestHomeDir\\worker-1 jar'
+        def line = '"C:\\Program Files\\Java\\jdk1.7/bin/java.exe"    -Dorg.gradle.daemon.idletimeout=120000 -Dorg.gradle.daemon.registry.base=C:\\some\\agent\\workspace\\build\\daemon -Dorg.gradle.native.dir=C:\\some\\agent\\workspace\\intTestHomeDir\\worker-1\\native -Dorg.gradle.deprecation.trace=true -Djava.io.tmpdir=C:\\some\\agent\\workspace\\subprojects\\osgi\\build\\tmp -Dfile.encoding=windows-1252 -Dorg.gradle.classloaderscope.strict=true -ea -ea "-Dorg.gradle.appname=gradle" -classpath "C:\\some\\agent\\workspace\\subprojects\\osgi\\build\\integ test\\bin\\..\\lib\\gradle-launcher-4.5.jar" org.gradle.launcher.GradleMain --init-script "C:\\some\\agent\\workspace\\subprojects\\osgi\\build\\tmp\\teŝt files\\OsgiPluginIntegrationSpec\\can_merge_manifests...archives_\\uz4kt\\reproducible-archives-init.gradle" --no-daemon --stacktrace --gradle-user-home C:\\some\\agent\\workspace\\intTestHomeDir\\worker-1 jar'
         def projectDir = 'C:\\some\\agent\\workspace'
 
         expect:

subprojects/build-cache/src/test/groovy/org/gradle/caching/internal/controller/DefaultBuildCacheControllerTest.groovy

@@ -79,7 +79,7 @@ class DefaultBuildCacheControllerTest extends Specification {
                 remotePush
             ),
             operations,
-            TestFiles.tmpDirTemporaryFileProvider(tmpDir.root),
+            TestFiles.tmpDirTemporaryFileProvider(tmpDir.testDirectory),
             false,
             false,
             disableRemoteOnError,

subprojects/build-cache/src/test/groovy/org/gradle/caching/local/internal/DirectoryBuildCacheServiceFactoryTest.groovy

@@ -44,7 +44,7 @@ class DirectoryBuildCacheServiceFactoryTest extends Specification {
     def fileStoreFactory = Mock(DirectoryBuildCacheFileStoreFactory)
     def cleanupActionDecorator = Mock(CleanupActionDecorator)
     def fileAccessTimeJournal = Mock(FileAccessTimeJournal)
-    def factory = new DirectoryBuildCacheServiceFactory(cacheRepository, globalScopedCache, resolver, fileStoreFactory, cleanupActionDecorator, fileAccessTimeJournal, TestFiles.tmpDirTemporaryFileProvider(temporaryFolder.root))
+    def factory = new DirectoryBuildCacheServiceFactory(cacheRepository, globalScopedCache, resolver, fileStoreFactory, cleanupActionDecorator, fileAccessTimeJournal, TestFiles.tmpDirTemporaryFileProvider(temporaryFolder.createDir("tmp")))
     def cacheBuilder = Stub(CacheBuilder)
     def config = Mock(DirectoryBuildCache)
     def buildCacheDescriber = new NoopBuildCacheDescriber()

subprojects/build-init/src/main/java/org/gradle/api/tasks/wrapper/Wrapper.java

@@ -419,7 +419,7 @@ public String getDistributionUrl() {
         if (distributionUrl != null) {
             return distributionUrl;
         } else if (gradleVersionResolver.getGradleVersion() != null) {
-            return locator.getDistributionFor(gradleVersionResolver.getGradleVersion(), distributionType.name().toLowerCase(Locale.ENGLISH)).toString();
+            return locator.getDistributionFor(gradleVersionResolver.getGradleVersion(), distributionType.name().toLowerCase(Locale.ENGLISH)).toASCIIString();
         } else {
             return null;
         }

subprojects/build-scan-performance/src/testFixtures/groovy/org/gradle/performance/AbstractBuildScanPluginPerformanceTest.groovy

@@ -29,7 +29,6 @@ import org.gradle.performance.measure.MeasuredOperation
 import org.gradle.performance.results.BaselineVersion
 import org.gradle.performance.results.BuildScanResultsStore
 import org.gradle.performance.results.CrossBuildPerformanceResults
-import org.gradle.test.fixtures.file.TestNameTestDirectoryProvider
 import org.junit.Rule
 import spock.lang.AutoCleanup
 import spock.lang.Shared
@@ -40,9 +39,6 @@ class AbstractBuildScanPluginPerformanceTest extends AbstractPerformanceTest {
 
     static String incomingDir = "../../incoming"
 
-    @Rule
-    TestNameTestDirectoryProvider temporaryFolder = new TestNameTestDirectoryProvider(getClass())
-
     @Rule
     PerformanceTestIdProvider performanceTestIdProvider = new PerformanceTestIdProvider()
 

subprojects/code-quality/src/integTest/groovy/org/gradle/api/plugins/quality/checkstyle/CheckstylePluginExternalDTDOptionIntegrationTest.groovy

@@ -17,6 +17,7 @@
 package org.gradle.api.plugins.quality.checkstyle
 
 import org.gradle.integtests.fixtures.AbstractIntegrationSpec
+import org.gradle.test.fixtures.file.DoesNotSupportNonAsciiPaths
 import spock.lang.Issue
 import spock.lang.See
 
@@ -25,6 +26,7 @@ import static org.gradle.api.plugins.quality.checkstyle.CheckstylePluginMultiPro
 
 @Issue("https://github.com/gradle/gradle/issues/21624")
 @See("https://checkstyle.sourceforge.io/config_system_properties.html#Enable_External_DTD_load")
+@DoesNotSupportNonAsciiPaths(reason = "https://github.com/checkstyle/checkstyle/issues/13012")
 class CheckstylePluginExternalDTDOptionIntegrationTest extends AbstractIntegrationSpec {
     def setup() {
         buildFile << """

subprojects/core/src/test/groovy/org/gradle/api/internal/file/BasicFileResolverTest.groovy

@@ -23,7 +23,7 @@ import spock.lang.Specification
 
 class BasicFileResolverTest extends Specification {
     @Rule TestNameTestDirectoryProvider tmpDir = new TestNameTestDirectoryProvider(getClass())
-    def baseDir = tmpDir.createDir("base-dir")
+    def baseDir = tmpDir.createDir("teŝt dir")
     def resolver = new BasicFileResolver(baseDir)
 
     def "converts relative path"() {
@@ -44,7 +44,7 @@ class BasicFileResolverTest extends Specification {
         def target = tmpDir.file("some-file")
 
         expect:
-        resolver.transform(target.toURI().toString()) == target
+        resolver.transform(target.toURI().toASCIIString()) == target
     }
 
     def "does not convert http URI"() {

subprojects/core/src/test/groovy/org/gradle/internal/classpath/ClasspathBuilderTest.groovy

@@ -27,7 +27,7 @@ import java.util.zip.ZipEntry
 class ClasspathBuilderTest extends Specification {
     @Rule
     TestNameTestDirectoryProvider tmpDir = new TestNameTestDirectoryProvider(ClasspathBuilderTest)
-    ClasspathBuilder builder = new ClasspathBuilder(TestFiles.tmpDirTemporaryFileProvider(tmpDir.root))
+    ClasspathBuilder builder = new ClasspathBuilder(TestFiles.tmpDirTemporaryFileProvider(tmpDir.createDir("tmp")))
 
     def "creates an empty jar"() {
         def file = tmpDir.file("thing.zip")

subprojects/core/src/test/groovy/org/gradle/internal/classpath/DefaultCachedClasspathTransformerTest.groovy

@@ -67,7 +67,7 @@ class DefaultCachedClasspathTransformerTest extends ConcurrentSpec {
     def cacheConfigurations = Stub(CacheConfigurationsInternal)
     def cacheFactory = new DefaultClasspathTransformerCacheFactory(usedGradleVersions, cacheConfigurations)
     def classpathWalker = new ClasspathWalker(TestFiles.fileSystem())
-    def classpathBuilder = new ClasspathBuilder(TestFiles.tmpDirTemporaryFileProvider(testDirectoryProvider.root))
+    def classpathBuilder = new ClasspathBuilder(TestFiles.tmpDirTemporaryFileProvider(testDirectoryProvider.createDir("tmp")))
     def fileSystemAccess = TestFiles.fileSystemAccess()
     def globalCacheLocations = Stub(GlobalCacheLocations)
     def fileLockManager = Stub(FileLockManager)

subprojects/core/src/test/groovy/org/gradle/process/internal/DefaultExecActionFactoryTest.groovy

@@ -35,7 +35,7 @@ class DefaultExecActionFactoryTest extends ConcurrentSpec {
     def instantiator = TestUtil.instantiatorFactory()
     def factory =
         DefaultExecActionFactory
-            .of(resolver, fileCollectionFactory, executorFactory, TestFiles.tmpDirTemporaryFileProvider(tmpDir.root))
+            .of(resolver, fileCollectionFactory, executorFactory, TestFiles.tmpDirTemporaryFileProvider(tmpDir.createDir("tmp")))
             .forContext()
             .withInstantiator(instantiator.decorateLenient())
             .withObjectFactory(TestUtil.objectFactory())

subprojects/core/src/test/groovy/org/gradle/process/internal/JavaExecHandleBuilderTest.groovy

@@ -36,7 +36,7 @@ import static java.util.Arrays.asList
 class JavaExecHandleBuilderTest extends Specification {
     @Rule
     final TestNameTestDirectoryProvider tmpDir = new TestNameTestDirectoryProvider(getClass())
-    final TemporaryFileProvider temporaryFileProvider = TestFiles.tmpDirTemporaryFileProvider(tmpDir.root)
+    final TemporaryFileProvider temporaryFileProvider = TestFiles.tmpDirTemporaryFileProvider(tmpDir.testDirectory)
     JavaExecHandleBuilder builder = new JavaExecHandleBuilder(
         TestFiles.resolver(),
         TestFiles.fileCollectionFactory(),

subprojects/dependency-management/build.gradle.kts

@@ -123,7 +123,7 @@ packageCycles {
 testFilesCleanup.reportOnly = true
 
 tasks.clean {
-    val testFiles = layout.buildDirectory.dir("tmp/test files")
+    val testFiles = layout.buildDirectory.dir("tmp/teŝt files")
     doFirst {
         // On daemon crash, read-only cache tests can leave read-only files around.
         // clean now takes care of those files as well

subprojects/dependency-management/src/integTest/groovy/org/gradle/integtests/resolve/ResolveConfigurationRepositoriesBuildOperationIntegrationTest.groovy

@@ -36,7 +36,7 @@ class ResolveConfigurationRepositoriesBuildOperationIntegrationTest extends Abst
         using m2
         buildFile << """
             apply plugin: 'java'
-            ${repoBlock.replaceAll('<<URL>>', mavenHttpRepo.uri.toString())}
+            ${repoBlock.replaceAll('<<URL>>', mavenHttpRepo.uri.toASCIIString())}
             task resolve {
                 def files = configurations.compileClasspath
                 doLast { files.files }
@@ -59,7 +59,7 @@ class ResolveConfigurationRepositoriesBuildOperationIntegrationTest extends Abst
         def repo1 = repos.first()
         repo1.remove('id')
         repo1 == augmentMapWithProperties(expectedRepo, [
-            URL: expectedRepo.name == 'MavenLocal' ? m2.mavenRepo().uri.toString() : mavenHttpRepo.uri.toString(),
+            URL: expectedRepo.name == 'MavenLocal' ? m2.mavenRepo().uri.toASCIIString() : mavenHttpRepo.uri.toASCIIString(),
             DIRS: [buildFile.parentFile.file('fooDir').absolutePath]
         ])
 

subprojects/dependency-management/src/test/groovy/org/gradle/api/internal/runtimeshaded/RuntimeShadedJarCreatorTest.groovy

@@ -58,7 +58,7 @@ class RuntimeShadedJarCreatorTest extends Specification {
             progressLoggerFactory,
             new ImplementationDependencyRelocator(RuntimeShadedJarType.API),
             new ClasspathWalker(TestFiles.fileSystem()),
-            new ClasspathBuilder(TestFiles.tmpDirTemporaryFileProvider(tmpDir.root))
+            new ClasspathBuilder(TestFiles.tmpDirTemporaryFileProvider(tmpDir.createDir("tmp")))
         )
     }
 

subprojects/file-collections/src/main/java/org/gradle/api/internal/file/FileOrUriNotationConverter.java

@@ -28,9 +28,12 @@
 import org.gradle.internal.typeconversion.TypeConversionException;
 
 import java.io.File;
+import java.io.UnsupportedEncodingException;
 import java.net.URI;
 import java.net.URISyntaxException;
 import java.net.URL;
+import java.net.URLDecoder;
+import java.nio.charset.StandardCharsets;
 import java.nio.file.Path;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
@@ -40,9 +43,6 @@ public class FileOrUriNotationConverter implements NotationConverter<Object, Obj
     private static final Pattern URI_SCHEME = Pattern.compile("([a-zA-Z][a-zA-Z0-9+-\\.]*:).+");
     private static final Pattern ENCODED_URI = Pattern.compile("%([0-9a-fA-F]{2})");
 
-    public FileOrUriNotationConverter() {
-    }
-
     public static NotationParser<Object, Object> parser() {
         return NotationParserBuilder
             .toType(Object.class)
@@ -122,27 +122,41 @@ public void convert(Object notation, NotationConvertResult<? super Object> resul
         }
     }
 
-    private boolean isWindowsRootDirectory(String scheme) {
+    private static boolean isWindowsRootDirectory(String scheme) {
         return scheme.length() == 2 && Character.isLetter(scheme.charAt(0)) && scheme.charAt(1) == ':' && OperatingSystem.current().isWindows();
     }
 
-    private void convertToUrl(String notationString, NotationConvertResult<? super Object> result) {
+    private static void convertToUrl(String notationString, NotationConvertResult<? super Object> result) {
         try {
             result.converted(new URI(notationString));
         } catch (URISyntaxException e) {
             throw new UncheckedIOException(e);
         }
     }
 
-    private String uriDecode(String path) {
+    private static String uriDecode(String path) {
+        try {
+            return URLDecoder.decode(path, StandardCharsets.UTF_8.name());
+        } catch (IllegalArgumentException e) {
+            return fallbackUrlDecode(path);
+        } catch (UnsupportedEncodingException e) {
+            throw new AssertionError(e);
+        }
+    }
+
+    /**
+     * Lenient legacy behavior to fall back to when URI cannot be normally parsed.
+     *
+     * TODO: Deprecate this
+     */
+    private static String fallbackUrlDecode(String path) {
         StringBuffer builder = new StringBuffer();
         Matcher matcher = ENCODED_URI.matcher(path);
         while (matcher.find()) {
             String val = matcher.group(1);
-            matcher.appendReplacement(builder, String.valueOf((char) (Integer.parseInt(val, 16))));
+            matcher.appendReplacement(builder, String.valueOf((char) Integer.parseInt(val, 16)));
         }
         matcher.appendTail(builder);
         return builder.toString();
     }
-
 }