Inherit encoding from parent process in worker daemons on Linux #25319
Conversation
lptr
force-pushed
the
lptr/workers/pass-encoding-to-worker-daemons
branch
from
ed2a48b
to
a5868cc
Compare
lptr
force-pushed
the
lptr/workers/pass-encoding-to-worker-daemons
branch
from
a5868cc
to
8bcc651
Compare
lptr
changed the title
| if (!OperatingSystem.current().isUnix()) { | ||
| return ImmutableMap.of(); | ||
| } | ||
| Map<String, Object> environment = forkOptions.getEnvironment(); |
There was a problem hiding this comment.
Is it possible that forkOptions.getEnvironment() returns immutable map, so removal of entries could throw an exception?
There was a problem hiding this comment.
If we think this is meaningful, we can try to improve things in a separate PR.
This PR might be backported as far as 8.1, so I intentionally kept it as small as I could.
There was a problem hiding this comment.
But if user passes immutable map to forOptions, then this will blow up. Or user can't set a map that is accessed here?
There was a problem hiding this comment.
Ah, I see what you mean. I thought you meant we should make it immutable. I think you're right, let me fix this.
There was a problem hiding this comment.
Actually, no, let's leave it as is. This code was already calling forkOptions.setEnvironment() in the constructor, which would have blown up with an ImmutableJavaForkOptions. So we aren't making things worse here.
This should be improved, but let's do that in a separate PR.
There was a problem hiding this comment.
I don't like the current semantic of that method: it returns a Map but potentially by modifying the original Map. Which means in some cases the assignment is not needed. That feels like a code smell to me. I would rather have a clear new Map being built always.
There was a problem hiding this comment.
Alright then. Should be fixed now.
This forces the test path to include non-ascii characters
lptr
force-pushed
the
lptr/workers/pass-encoding-to-worker-daemons
branch
from
0bbb3f9
to
fb2690c
Compare
lptr
changed the title
|
@bot-gradle cancel |
|
I've cancelled the pre-tested commit build. |
|
@bot-gradle test and merge please. |
|
Your PR is queued. See the queue page for details. |
… 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>
The
LANG,LANGUAGEandLC_*(includingLC_ALL) environment variables are now passed to worker API daemon processes on Unix. This fixes a problem where on Linux worker daemons would try to access the file system with the defaultASCIIencoding.We are passing as few parameters as possible, though, to avoid making worker processes unnecessarily incompatible.
There is also a
@SupportsNonAsciiPathsannotation for integration tests that should work when they are executed with file paths containing non-ASCII characters. These tests are executed in theteșt files(notice the U+0219 character) instead of the usualtest filesfolder.(The intention is to run most integration tests with such a non-ASCII path to shake out more bugs, and annotate the ones that cannot parse non-ASCII paths with
@DoesNotSupportNonAsciiPaths. But this is a next step.)Fixes #25198
Fixes #13843