New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor uses & creation of temp files/dirs to use unified API #15240
Refactor uses & creation of temp files/dirs to use unified API #15240
Conversation
68ae1ea
to
7797fc0
Compare
7797fc0
to
0871c30
Compare
0871c30
to
29d8ed9
Compare
29d8ed9
to
6c082d7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started reviewing this and left a bunch of comments, but the single change here is way too big, and it's hard to tell what belongs where. It would be great if you could split this up into several smaller commits.
| implementation(project(":base-services")) | ||
|
|
||
| implementation(libs.inject) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think working with temporary files requires a separate subproject. Indeed, we have the :files subproject that already contains classes in the org.gradle.internal.file package. I think these should go there, perhaps under org.gradle.internal.file.temp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code needed to make its way into a variety of places, like workers and such, and I didn't want to be adding more dependencies than were strictly necessary when doing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think :files is already a dependency of everything where you'd need this. What does not depend on it that still needs temporary files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The projects jvm-services & native don't depend upon the entirety of files and I didn't want to create a coupling here on more than was necessary. My goal here was to apply the single-responsibility principle so that downstream consumers are coupled to the fewest dependencies. Thus allowing Gradle to continue to be able to parallelize the compilation of these components. If I were to introduce this explicit coupling, compilation wouldn't be able to happen in parallel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very much commendable, however with our current setup adding a new JAR adds some non-trivial costs, too, both at build time and at runtime. The most significant of these is the lengthening of the Windows classpath, which causes innumerable pains. We should fix this for good at some point by combining our JARs into a single fat one or something, but we don't seem to be getting around to doing this.
In any case I don't think parallel compilation for these three classes is a major issue that would warrant a separate subproject by itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran this by @big-guy and he gave me guidance to continue with the separate project for now. You're not wrong about the issues here with the the windows paths. We've decided we can always re-combine these if we have issues moving forward.
subprojects/file-temp/src/main/java/org/gradle/api/internal/file/TemporaryFileProvider.java
Outdated
Show resolved
Hide resolved
...jects/file-temp/src/main/java/org/gradle/api/internal/file/DefaultTemporaryFileProvider.java
Show resolved
Hide resolved
| }); | ||
| } | ||
|
|
||
| private TmpDirTemporaryFileProvider(final Factory<File> tempDirProvider) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We now use Tmp (in TmpDirTemporaryFileProvider), Temp (in TempFiles) and Temporary to name these types. This line has all three variations.
I think it would be great to standardize on one way instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hopefully this won't last much longer.
| import java.io.File; | ||
|
|
||
| @ServiceScope(Scopes.UserHome.class) | ||
| public class TmpDirTemporaryFileProvider extends DefaultTemporaryFileProvider { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could call this a DirectoryBasedTemporaryFileProvider to avoid repeating "temporary".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this different to DefaultTemporaryFileProvider?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DefaultTemporaryFileProvider is for the underlying logic for creating the temporary files. The TmpDirTemporaryFileProvider is an implementation detail that the files are actually coming from the system temporary directory. After this PR is merged I'm going to be working on trying to get rid of the actual uses of this class.
TmpDir - It's coming from the system temp directory.
TemporaryFileProvider - It's implementing the TemporaryFileProvider interface.
The plan is to move Gradle to create and use a temporary directory under the Gradle User Home moving forward.
subprojects/core/src/main/java/org/gradle/internal/service/scopes/BasicGlobalScopeServices.java
Outdated
Show resolved
Hide resolved
subprojects/language-groovy/src/main/java/org/gradle/api/tasks/javadoc/Groovydoc.java
Outdated
Show resolved
Hide resolved
66ca5d1
to
5f24f7c
Compare
5f24f7c
to
1b29c1e
Compare
f28bc4a
to
22ec917
Compare
22ec917
to
07c7a20
Compare
07c7a20
to
2a1581f
Compare
2a1581f
to
ded10e1
Compare
...jects/file-temp/src/main/java/org/gradle/api/internal/file/DefaultTemporaryFileProvider.java
Show resolved
Hide resolved
subprojects/base-services/src/main/java/org/gradle/internal/file/TempFiles.java
Show resolved
Hide resolved
2460e76
to
746b6d5
Compare
This centralization allows unified control over temporary file & directory creation. The goal is to drive all temp directory creation logic through a set of common logical components. Doing so allows us to make our temp directory usage relocatable.
746b6d5
to
00ea118
Compare
|
@bot-gradle test QuickFeedback |
|
OK, I've already triggered QuickFeedback build for you. |
|
@bot-gradle test this please |
|
OK, I've already triggered ReadyForMerge build for you. |
Related https://github.com/gradle/gradle-private/issues/3184