Skip to content
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

Lazily acquire JDK storage locations #2263

Merged
merged 12 commits into from
May 12, 2022
Merged

Lazily acquire JDK storage locations #2263

merged 12 commits into from
May 12, 2022

Conversation

CRogers
Copy link
Contributor

@CRogers CRogers commented May 10, 2022

Before this PR

You must know about all the JDK versions you need to set before the baseline-java-versions plugin uses them. This can make being a lazy as possible harder, and complicates the logic slightly.

After this PR

==COMMIT_MSG==
More lazily acquire JDK storage locations
==COMMIT_MSG==

Possible downsides?

@CRogers CRogers requested a review from carterkozak May 10, 2022 12:21
@@ -31,7 +33,8 @@ public class BaselineJavaVersionsExtension {
private final Property<JavaLanguageVersion> libraryTarget;
private final Property<JavaLanguageVersion> distributionTarget;
private final Property<JavaLanguageVersion> runtime;
private final MapProperty<JavaLanguageVersion, JavaInstallationMetadata> jdks;
private final LazilyConfiguredMapping<JavaLanguageVersion, AtomicReference<JavaInstallationMetadata>> jdks =
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why use an AtomicReference here? I wanted to make a general type I could reuse in gradle-jdks to configure the versions/distributions, which would involve modifying extension objects in the regular gradle style. So we use an AtomicReference here just to make it act like a box you can store data in.

import java.util.function.Supplier;
import org.gradle.api.Action;

public final class LazilyConfiguredMapping<K, V> {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would hope to reuse this in gradle-jdks to have everything be ETE lazy.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps move to the extensions package and make package private?


public final class LazilyConfiguredMapping<K, V> {
private final Supplier<V> valueFactory;
private final List<LazyValues<K, V>> values = new ArrayList<>();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might have been easier to set this as just a single function you have to replace - but I'm concerned that just leads to everything becoming confusing if you "override" the value in custom gradle for example to test a new JDK major version that hasn't been fully released yet.

}

public interface LazyJdks {
Optional<JavaInstallationMetadata> jdkFor(JavaLanguageVersion javaLanguageVersion);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is basically making it fully lazy.

@bulldozer-bot bulldozer-bot bot merged commit fd1c998 into develop May 12, 2022
@bulldozer-bot bulldozer-bot bot deleted the callumr/lazier branch May 12, 2022 10:53
@svc-autorelease
Copy link
Collaborator

Released 4.133.0

This was referenced May 12, 2022
bulldozer-bot bot pushed a commit to palantir/witchcraft-api that referenced this pull request May 12, 2022
###### _excavator_ is a bot for automating changes across repositories.

Changes produced by the roomba/latest-baseline-oss check.

# Release Notes
## 4.132.0
| Type | Description | Link |
| ---- | ----------- | ---- |
| Improvement | Safety analysis checks superinterfaces (in addition to superclasses) | palantir/gradle-baseline#2267 |


## 4.133.0
| Type | Description | Link |
| ---- | ----------- | ---- |
| Fix | More lazily acquire JDK storage locations | palantir/gradle-baseline#2263 |


## 4.134.0
| Type | Description | Link |
| ---- | ----------- | ---- |
| Fix | Fix concurrency issue in LazilyConfiguredMapping | palantir/gradle-baseline#2268 |



To enable or disable this check, please contact the maintainers of Excavator.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants