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.

Sign up for GitHub

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

Jump to bottom

Simplify consumption of Shared Build Services #16168

Closed
Vampire opened this issue Feb 16, 2021 · 4 comments · Fixed by #22206
Closed

Simplify consumption of Shared Build Services #16168

Vampire opened this issue Feb 16, 2021 · 4 comments · Fixed by #22206
Assignees
@abstratt
Labels
a:feature A new functionality @configuration-cache in:build-services Shared Build Services
Milestone
8.1 RC1

Comments

@Vampire
Copy link
Contributor

Vampire commented Feb 16, 2021

If you have registered a shared build service with 6.8.2 like desribed here: https://docs.gradle.org/current/userguide/build_services.html, you still have to manually wire that build service into the classes where you need to use it.

Would be nice if you could instead @Inject those services just like you inject the built in services.
@Vampire Vampire added a:feature A new functionality from:contributor labels Feb 16, 2021
@Vampire Vampire mentioned this issue Feb 16, 2021
Closed
@jbartok jbartok added in:build-services Shared Build Services and removed in:core labels Nov 26, 2021
@abstratt abstratt mentioned this issue Aug 31, 2022
Closed
abstratt added a commit that referenced this issue Aug 31, 2022
@abstratt
Spike on shared build service injection by name
da684d6 
Issue: #16168
abstratt added a commit that referenced this issue Sep 9, 2022
@abstratt
Spike on shared build service injection by name
8e3faae 
Names are optional though

Issue: #16168
abstratt added a commit that referenced this issue Sep 23, 2022
@abstratt
Spike on shared build service injection by name
076b3c1 
Names are optional though

Issue: #16168
@abstratt abstratt mentioned this issue Sep 27, 2022
Merged
abstratt added a commit that referenced this issue Oct 6, 2022
@abstratt
Implement injectable shared build services
974ad10 
Fixes: #16168
abstratt added a commit that referenced this issue Oct 24, 2022
@abstratt
Implement injectable shared build services
0f21aa3 
Fixes: #16168
abstratt added a commit that referenced this issue Nov 7, 2022
@abstratt
Implement injectable shared build services
debfd3c 
Fixes: #16168
abstratt added a commit that referenced this issue Nov 10, 2022
@abstratt
Implement injectable shared build services
28f2b87 
Fixes: #16168
abstratt added a commit that referenced this issue Nov 11, 2022
@abstratt
Implement injectable shared build services
45d04bf 
Fixes: #16168
abstratt added a commit that referenced this issue Nov 14, 2022
@abstratt
Honor @optional vs @ServiceReference
6972591 
* Clean up DefaultTaskRequiredServices

Remove support for visiting explicity registered services as there is
no use case for that yet.

* Add test case for service refs in @nested beans

Allow services of type BuildServiceRegistry to be available for
injection into instances of type BuildService.

Also add failing test cases for service refs in artifact transform
parameters and other service parameters.

Co-authored-by: Rodrigo Oliveira <rodrigo@gradle.com>

Issue: #16168
abstratt added a commit that referenced this issue Nov 14, 2022
@abstratt @bamboo
Avoid unwrapping a property value eagerly
8ef2125 
If a service reference visitor needs the provider, it can unwrap the
provider reference itself.

Make `ConsumedBuildServiceProvider` thread-safe

Issue: #16168

Co-authored-by: Rodrigo Oliveira <rodrigo@gradle.com>
abstratt added a commit that referenced this issue Dec 15, 2022
@abstratt
Prevent service references from being finalized
dab77a3 
There are expetactions in place that shared build services are
held by non-finalized properties, so they retain BuildServiceProviders
(which hold metadata on shared build services) instead of having them
resolved to fixed value providers.

This change basically ensures BuildServiceProviders are preserved
even if the property using them is finalized.

Issue: #16168
abstratt added a commit that referenced this issue Dec 15, 2022
@abstratt
Automatically assign unnamed service refs by type
89a2b93 
When the number of services of the required type == 1.

Use immutable collection

Add test for avoiding ambiguous service lookup

Issue: #16168
bot-gradle added a commit that referenced this issue Dec 15, 2022
@bot-gradle @abstratt
Merge pull request #22987 Prevent build services from being instantia…
6561370 
…ted during finalization

There are expetactions in place that shared build services are held by non-finalized properties, so they retain BuildServiceProviders (which hold metadata on shared build services) instead of having them resolved to fixed value providers.

This change basically ensures BuildServiceProviders are preserved even if the property using them is finalized.

Issue: #16168

### Rationale

This addresses issues related to using service references being finalized (and leading to BuildServiceProviders being replaced by fixed value providers, preventing us from accessing service metadata that is only available to instances of BuildServiceProvider). Here are my findings:

`@ServiceReference` properties were implemented as validated (regular @internal properties are not).

When validating a property, [we unpack its value](https://github.com/gradle/gradle/blob/734c91716ed48229bf05bf8d3d593476da32257d/subprojects/core/src/main/java/org/gradle/api/internal/tasks/properties/AbstractValidatingProperty.java#L53), which [resolves providers](https://github.com/gradle/gradle/blob/e56d98ee8b46ecd74c247076e0a1fc0183280de1/subprojects/model-core/src/main/java/org/gradle/util/internal/DeferredUtil.java#L59).

If a property is marked as `finalizeValueOnRead`, any attempt to read it (including during validtion) will finalize its value.

```
finalValue:133, DefaultProperty (org.gradle.api.internal.provider)
finalizeNow:245, AbstractProperty (org.gradle.api.internal.provider)
beforeRead:239, AbstractProperty (org.gradle.api.internal.provider)
calculateOwnValue:135, AbstractProperty (org.gradle.api.internal.provider)
getOrNull:98, AbstractMinimalProvider (org.gradle.api.internal.provider)
resolve:27, ProviderResolutionStrategy$1 (org.gradle.api.internal.provider)
unpack:59, DeferredUtil (org.gradle.util.internal)
unpackOrNull:49, DeferredUtil (org.gradle.util.internal)
validate:53, AbstractValidatingProperty (org.gradle.api.internal.tasks.properties)
validate:139, DefaultTaskProperties (org.gradle.api.internal.tasks.properties)
validate:466, TaskExecution (org.gradle.api.internal.tasks.execution)
execute:69, ValidateStep (org.gradle.internal.execution.steps)
execute:71, CaptureStateBeforeExecutionStep (org.gradle.internal.execution.steps)
executeWithNonEmptySources:177, SkipEmptyWorkStep (org.gradle.internal.execution.steps)
execute:81, SkipEmptyWorkStep (org.gradle.internal.execution.steps)
execute:32, RemoveUntrackedExecutionStateStep (org.gradle.internal.execution.steps)
execute:38, MarkSnapshottingInputsStartedStep (org.gradle.internal.execution.steps.legacy)
execute:36, LoadPreviousExecutionStateStep (org.gradle.internal.execution.steps)
execute:75, CleanupStaleOutputsStep (org.gradle.internal.execution.steps)
lambda$execute$0:32, AssignWorkspaceStep (org.gradle.internal.execution.steps)
withWorkspace:287, TaskExecution$4 (org.gradle.api.internal.tasks.execution)
execute:30, AssignWorkspaceStep (org.gradle.internal.execution.steps)
execute:37, IdentityCacheStep (org.gradle.internal.execution.steps)
execute:42, IdentifyStep (org.gradle.internal.execution.steps)
execute:64, DefaultExecutionEngine$1 (org.gradle.internal.execution.impl)
executeIfValid:146, ExecuteActionsTaskExecuter (org.gradle.api.internal.tasks.execution)
execute:135, ExecuteActionsTaskExecuter (org.gradle.api.internal.tasks.execution)
execute:46, FinalizePropertiesTaskExecuter (org.gradle.api.internal.tasks.execution)
execute:51, ResolveTaskExecutionModeExecuter (org.gradle.api.internal.tasks.execution)
```

The reason service reference properties are marked for finalization on read is that before validation, we request all properties that are declared as validatable to finalize on next get in [FinalizePropertiesTaskExecuter](https://github.com/gradle/gradle/blob/c68724158ac7c5d93478e584fa714169c6ad63eb/subprojects/core/src/main/java/org/gradle/api/internal/tasks/execution/FinalizePropertiesTaskExecuter.java#L43). Basically, right now, if a property is validatable, it is also lifecycle aware, and hence is marked as finalizeOnNextGet().

```
finalizeOnNextGet:406, AbstractProperty$NonFinalizedValue (org.gradle.api.internal.provider)
implicitFinalizeValue:211, AbstractProperty (org.gradle.api.internal.provider)
maybeFinalizeValue:133, AbstractNestedRuntimeBeanNode$BeanPropertyValue (org.gradle.internal.properties.bean)
prepareValue:65, AbstractValidatingProperty (org.gradle.api.internal.tasks.properties)
execute:43, FinalizePropertiesTaskExecuter (org.gradle.api.internal.tasks.execution)
execute:51, ResolveTaskExecutionModeExecuter (org.gradle.api.internal.tasks.execution)
```

Long story short, we prematurely finalize service references (if they are marked as `finalizeOnValueRead()`), but not old school shared-build-service-typed properties (as they are @internal and skipped by validation).

A couple of possible solutions:
1. implement finalization of BuildServiceProviders (which requires overriding [`ProviderInternal#withFinalValue`](https://github.com/gradle/gradle/blob/46f2897929ed721c0a2f6505283843b6a3b45623/subprojects/model-core/src/main/java/org/gradle/api/internal/provider/ProviderInternal.java#L134))
1. make ServiceReference properties validatable but not lifecycle-aware (up to now, both sets where always identical, "validatable" implied "lifecycle aware").
1. a third one would be to undo the design decision of service references validatable.

The first option also ensures that even if a user calls `#finalizeValueOnRead` we could still can handle it in a way that made sense for shared build service providers (like if).

The second one misses that case - however, note that today it is already illegal to call `#finalizeValue()`on a property (as it causes the original BSP provider to be replaced with fixed one, and in multiple places we require a BuildService property to hold on to a BuildServiceProvider, not a general provider that returns a build service).

This change set implements #1 above.

Co-authored-by: Rafael Chaves <rchaves@gradle.com>
@abstratt abstratt modified the milestones: 8.0 RC1, 8.1 RC1 Dec 20, 2022
@abstratt
Copy link
Member

abstratt commented Dec 20, 2022 •
edited
Loading

The code will be in 8.0, but we want to allow us more time to tweak the API if needed before we advertise this feature, so it will only be documented for 8.1.

@abstratt
Copy link
Member

See demo project at:

https://github.com/abstratt/simplified-shared-build-service-demo

See spec at: https://docs.google.com/document/d/15dlxPKI0SZvmwgoMLtdGjlr4DbXp1VqMOzXpLEEWgj4/view#

@abstratt abstratt mentioned this issue Jan 2, 2023
Closed
@abstratt
Copy link
Member

abstratt commented Jan 2, 2023

This went into 8.0. Documentation and promotion will be covered by #23362.

@abstratt abstratt closed this as completed Jan 2, 2023
This was referenced Apr 17, 2023
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@abstratt abstratt

Labels
a:feature A new functionality @configuration-cache in:build-services Shared Build Services
Projects
Stable Configuration Cache
Status: Done
Milestone
8.1 RC1
Development

Successfully merging a pull request may close this issue.

Implement injectable shared build services gradle/gradle
6 participants
@eskatos @Vampire @jjohannes @abstratt @jbartok @bot-gradle