-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Add initializer for in-process Gradle executor #29142
base: master
Are you sure you want to change the base?
Conversation
void start() { | ||
if (GradleContextualExecuter.embedded ) { | ||
AbstractGradleExecuter.GLOBAL_SERVICES.get(ObjectFactory) | ||
} |
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.
Can we add some more comments about why this is happening only in embedded mode, and why this specific service is being queried?
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.
Is the Javadoc on the class enough for describing why it only happens for embedded mode? I can add something here why we query ObjectFactory
.
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 left some more comments.
@@ -115,6 +116,7 @@ public abstract class AbstractGradleExecuter implements GradleExecuter, Resettab | |||
.build(); | |||
|
|||
private static final JvmVersionDetector JVM_VERSION_DETECTOR = GLOBAL_SERVICES.get(JvmVersionDetector.class); | |||
private static final ObjectFactory objectFactory = GLOBAL_SERVICES.get(ObjectFactory.class); |
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.
Isn't this an unused field now? Can we clarify with a comment?
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 was a leftover, I removed 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.
Thanks! Comments really make it clear.
* may have a test implementation instead of the real thing when a unit test runs first. | ||
*/ | ||
@Issue("https://github.com/gradle/gradle-private/issues/3534") | ||
public class InProcessGradleExecutorInitialization implements IGlobalExtension { |
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.
💅 maybe reference this class in the InProcessGradleExecutor javadocs too? Global extensions are not something used often in our codebase (in fact, I've learned about them right now), and the extra visibility can save some time in the future for people figuring out how the executer is being initialized.
Shall we merge this? |
@lptr We can't merge this since I didn't fix the build problems, yet. Hope to get to that this week. |
Avoiding strange static state caused by unit tests that causes flaky test failures.
This was a leftover from some tryout
ca3e1d4
to
9d3bc5b
Compare
Use the project dependency on the current project instead.
Avoiding strange static state caused by unit tests that causes flaky test failures.
The real culprit seems to be the reuse of the cache inside the
AsmBackedClassGenerator
:gradle/subprojects/model-core/src/main/java/org/gradle/internal/instantiation/generator/AsmBackedClassGenerator.java
Lines 173 to 179 in d700905
If some test (e.g.
WorkerProcessIntegrationTest
) creates anAsmBackedClassGenerator
with theTestCrossBuildInMemoryCacheFactory
first, then all other tests will get aTestCache
as the cache implementation. TheTestCache
is not thread-safe, and theConcurrentClassDecorationSpec
would break.Making
TestCache
thread-safe fixes the immediate breakage of theConcurrentClassDecorationSpec
, but this is only a symptom and doesn't address the root cause. So we add the initializer for Global services instead so we don't get into the situation.