From 6abcc3fa4a01f276100955f0761096d705237be6 Mon Sep 17 00:00:00 2001 From: "Rodrigo B. de Oliveira" Date: Mon, 2 Mar 2020 18:34:17 -0300 Subject: [PATCH 01/13] Reinstate `PropertiesLoadingSettingsProcessor` To ensure `GradleProperties` is loaded before processing the settings scripts of any included builds. The integration with instant execution is mediated by the newly introduced `GradlePropertiesController` service which guarantees `GradleProperties` is loaded only once during a build and from the correct location. Fixes #12381 --- ...ositeBuildPropertiesIntegrationTest.groovy | 59 +++++++++ .../DefaultGradlePropertiesController.java | 114 ++++++++++++++++++ .../GradlePropertiesController.java | 45 +++++++ .../PropertiesLoadingSettingsProcessor.java | 47 ++++++++ .../initialization/SettingsFactory.java | 2 +- .../service/scopes/BuildScopeServices.java | 42 ++++--- ...faultGradlePropertiesControllerTest.groovy | 101 ++++++++++++++++ .../DefaultInstantExecution.kt | 10 +- 8 files changed, 403 insertions(+), 17 deletions(-) create mode 100644 subprojects/composite-builds/src/integTest/groovy/org/gradle/integtests/composite/CompositeBuildPropertiesIntegrationTest.groovy create mode 100644 subprojects/core/src/main/java/org/gradle/initialization/DefaultGradlePropertiesController.java create mode 100644 subprojects/core/src/main/java/org/gradle/initialization/GradlePropertiesController.java create mode 100644 subprojects/core/src/main/java/org/gradle/initialization/PropertiesLoadingSettingsProcessor.java create mode 100644 subprojects/core/src/test/groovy/org/gradle/initialization/DefaultGradlePropertiesControllerTest.groovy diff --git a/subprojects/composite-builds/src/integTest/groovy/org/gradle/integtests/composite/CompositeBuildPropertiesIntegrationTest.groovy b/subprojects/composite-builds/src/integTest/groovy/org/gradle/integtests/composite/CompositeBuildPropertiesIntegrationTest.groovy new file mode 100644 index 000000000000..2fe408f77374 --- /dev/null +++ b/subprojects/composite-builds/src/integTest/groovy/org/gradle/integtests/composite/CompositeBuildPropertiesIntegrationTest.groovy @@ -0,0 +1,59 @@ +/* + * Copyright 2020 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.gradle.integtests.composite + +import org.gradle.integtests.fixtures.AbstractIntegrationSpec +import org.gradle.integtests.fixtures.executer.GradleContextualExecuter +import spock.lang.IgnoreIf + +@IgnoreIf({ GradleContextualExecuter.isInstant() }) +class CompositeBuildPropertiesIntegrationTest extends AbstractIntegrationSpec { + + def "included build properties take precedence over root build properties"() { + given: + createDir("included") { + file("gradle.properties") << """ + theProperty=included + """ + file("settings.gradle") << """ + println("included settings script: " + theProperty) + """ + file("build.gradle") << """ + println("included build script: " + theProperty) + """ + } + file("gradle.properties") << """ + theProperty=root + """ + settingsFile << """ + includeBuild 'included' + println("root settings script: " + theProperty) + """ + buildFile << """ + println("root build script: " + theProperty) + """ + + when: + run('help') + + then: + outputContains("root settings script: root") + outputContains("root build script: root") + outputContains("included settings script: included") + outputContains("included build script: included") + } +} diff --git a/subprojects/core/src/main/java/org/gradle/initialization/DefaultGradlePropertiesController.java b/subprojects/core/src/main/java/org/gradle/initialization/DefaultGradlePropertiesController.java new file mode 100644 index 000000000000..79856430eaf4 --- /dev/null +++ b/subprojects/core/src/main/java/org/gradle/initialization/DefaultGradlePropertiesController.java @@ -0,0 +1,114 @@ +/* + * Copyright 2020 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.gradle.initialization; + +import org.gradle.api.internal.properties.GradleProperties; + +import javax.annotation.Nullable; +import java.io.File; +import java.util.Map; + +public class DefaultGradlePropertiesController implements GradlePropertiesController { + + private State state = new NotLoaded(); + private final GradleProperties sharedGradleProperties = new SharedGradleProperties(); + private final IGradlePropertiesLoader propertiesLoader; + + public DefaultGradlePropertiesController(IGradlePropertiesLoader propertiesLoader) { + this.propertiesLoader = propertiesLoader; + } + + @Override + public GradleProperties getGradleProperties() { + return sharedGradleProperties; + } + + @Override + public void loadGradlePropertiesFrom(File settingsDir) { + state = state.loadGradlePropertiesFrom(settingsDir); + } + + private class SharedGradleProperties implements GradleProperties { + + @Nullable + @Override + public String find(String propertyName) { + return gradleProperties().find(propertyName); + } + + @Override + public Map mergeProperties(Map properties) { + return gradleProperties().mergeProperties(properties); + } + + private GradleProperties gradleProperties() { + return state.getGradleProperties(); + } + } + + private interface State { + + GradleProperties getGradleProperties(); + + State loadGradlePropertiesFrom(File settingsDir); + } + + private class NotLoaded implements State { + + @Override + public GradleProperties getGradleProperties() { + throw new IllegalStateException("GradleProperties has not been loaded yet."); + } + + @Override + public State loadGradlePropertiesFrom(File settingsDir) { + return new Loaded( + propertiesLoader.loadGradleProperties(settingsDir), + settingsDir + ); + } + } + + private static class Loaded implements State { + + private final GradleProperties gradleProperties; + private final File propertiesDir; + + public Loaded(GradleProperties gradleProperties, File propertiesDir) { + this.gradleProperties = gradleProperties; + this.propertiesDir = propertiesDir; + } + + @Override + public GradleProperties getGradleProperties() { + return gradleProperties; + } + + @Override + public State loadGradlePropertiesFrom(File settingsDir) { + if (!propertiesDir.equals(settingsDir)) { + throw new IllegalStateException( + String.format( + "GradleProperties has already been loaded from '%s' and cannot be loaded from '%s'.", + propertiesDir, settingsDir + ) + ); + } + return this; + } + } +} diff --git a/subprojects/core/src/main/java/org/gradle/initialization/GradlePropertiesController.java b/subprojects/core/src/main/java/org/gradle/initialization/GradlePropertiesController.java new file mode 100644 index 000000000000..a436c5e5ca62 --- /dev/null +++ b/subprojects/core/src/main/java/org/gradle/initialization/GradlePropertiesController.java @@ -0,0 +1,45 @@ +/* + * Copyright 2020 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.gradle.initialization; + +import org.gradle.api.internal.properties.GradleProperties; + +import java.io.File; + +/** + * Controls the state (not loaded / loaded) of the attached {@link GradleProperties} instance + * so that the set of Gradle properties is deterministically loaded only once per build. + */ +public interface GradlePropertiesController { + + /** + * The {@link GradleProperties} instance attached to this service. + */ + GradleProperties getGradleProperties(); + + /** + * Loads the immutable set of {@link GradleProperties} from the given directory and + * makes it available to the build. + * + * This method should be called only once per build but multiple calls with the + * same argument are allowed. + * + * @param settingsDir directory where to look for the {@code gradle.properties} file + * @throws IllegalStateException if called with a different argument in the same build + */ + void loadGradlePropertiesFrom(File settingsDir); +} diff --git a/subprojects/core/src/main/java/org/gradle/initialization/PropertiesLoadingSettingsProcessor.java b/subprojects/core/src/main/java/org/gradle/initialization/PropertiesLoadingSettingsProcessor.java new file mode 100644 index 000000000000..675567f844b7 --- /dev/null +++ b/subprojects/core/src/main/java/org/gradle/initialization/PropertiesLoadingSettingsProcessor.java @@ -0,0 +1,47 @@ +/* + * Copyright 2020 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.gradle.initialization; + +import org.gradle.StartParameter; +import org.gradle.api.internal.GradleInternal; +import org.gradle.api.internal.SettingsInternal; +import org.gradle.api.internal.initialization.ClassLoaderScope; + +public class PropertiesLoadingSettingsProcessor implements SettingsProcessor { + + private final SettingsProcessor delegate; + private final GradlePropertiesController gradlePropertiesController; + + public PropertiesLoadingSettingsProcessor( + SettingsProcessor delegate, + GradlePropertiesController gradlePropertiesController + ) { + this.delegate = delegate; + this.gradlePropertiesController = gradlePropertiesController; + } + + @Override + public SettingsInternal process( + GradleInternal gradle, + SettingsLocation settingsLocation, + ClassLoaderScope buildRootClassLoaderScope, + StartParameter startParameter + ) { + gradlePropertiesController.loadGradlePropertiesFrom(settingsLocation.getSettingsDir()); + return delegate.process(gradle, settingsLocation, buildRootClassLoaderScope, startParameter); + } +} diff --git a/subprojects/core/src/main/java/org/gradle/initialization/SettingsFactory.java b/subprojects/core/src/main/java/org/gradle/initialization/SettingsFactory.java index 1ed1bc5f5265..b1c9c5748d76 100644 --- a/subprojects/core/src/main/java/org/gradle/initialization/SettingsFactory.java +++ b/subprojects/core/src/main/java/org/gradle/initialization/SettingsFactory.java @@ -18,13 +18,13 @@ import org.gradle.StartParameter; import org.gradle.api.internal.DynamicObjectAware; -import org.gradle.internal.extensibility.ExtensibleDynamicObject; import org.gradle.api.internal.GradleInternal; import org.gradle.api.internal.SettingsInternal; import org.gradle.api.internal.initialization.ClassLoaderScope; import org.gradle.api.internal.initialization.ScriptHandlerFactory; import org.gradle.api.internal.initialization.ScriptHandlerInternal; import org.gradle.groovy.scripts.ScriptSource; +import org.gradle.internal.extensibility.ExtensibleDynamicObject; import org.gradle.internal.metaobject.DynamicObject; import org.gradle.internal.reflect.Instantiator; import org.gradle.internal.service.scopes.ServiceRegistryFactory; diff --git a/subprojects/core/src/main/java/org/gradle/internal/service/scopes/BuildScopeServices.java b/subprojects/core/src/main/java/org/gradle/internal/service/scopes/BuildScopeServices.java index 6a2c718be17e..2ac307a395b5 100644 --- a/subprojects/core/src/main/java/org/gradle/internal/service/scopes/BuildScopeServices.java +++ b/subprojects/core/src/main/java/org/gradle/internal/service/scopes/BuildScopeServices.java @@ -121,10 +121,12 @@ import org.gradle.initialization.ClassLoaderScopeListeners; import org.gradle.initialization.ClassLoaderScopeRegistry; import org.gradle.initialization.DefaultClassLoaderScopeRegistry; +import org.gradle.initialization.DefaultGradlePropertiesController; import org.gradle.initialization.DefaultGradlePropertiesLoader; import org.gradle.initialization.DefaultProjectDescriptorRegistry; import org.gradle.initialization.DefaultSettingsLoaderFactory; import org.gradle.initialization.DefaultSettingsPreparer; +import org.gradle.initialization.GradlePropertiesController; import org.gradle.initialization.IGradlePropertiesLoader; import org.gradle.initialization.InitScriptHandler; import org.gradle.initialization.InstantiatingBuildLoader; @@ -133,6 +135,7 @@ import org.gradle.initialization.ProjectAccessListener; import org.gradle.initialization.ProjectDescriptorRegistry; import org.gradle.initialization.ProjectPropertySettingBuildLoader; +import org.gradle.initialization.PropertiesLoadingSettingsProcessor; import org.gradle.initialization.RootBuildCacheControllerSettingsProcessor; import org.gradle.initialization.ScriptEvaluatingSettingsProcessor; import org.gradle.initialization.SettingsEvaluatedCallbackFiringSettingsProcessor; @@ -271,6 +274,18 @@ protected IsolatedAntBuilder createIsolatedAntBuilder(ClassPathRegistry classPat return new DefaultIsolatedAntBuilder(classPathRegistry, classLoaderFactory, moduleRegistry); } + protected GradleProperties createGradleProperties( + GradlePropertiesController gradlePropertiesController + ) { + return gradlePropertiesController.getGradleProperties(); + } + + protected GradlePropertiesController createGradlePropertiesController( + IGradlePropertiesLoader propertiesLoader + ) { + return new DefaultGradlePropertiesController(propertiesLoader); + } + protected IGradlePropertiesLoader createGradlePropertiesLoader() { return new DefaultGradlePropertiesLoader((StartParameterInternal) get(StartParameter.class)); } @@ -295,13 +310,6 @@ protected ProviderFactory createProviderFactory(ValueSourceProviderFactory value return new DefaultProviderFactory(valueSourceProviderFactory); } - protected GradleProperties createGradleProperties( - IGradlePropertiesLoader propertiesLoader, - BuildLayout buildLayout - ) { - return propertiesLoader.loadGradleProperties(buildLayout.getRootDirectory()); - } - protected ActorFactory createActorFactory() { return new DefaultActorFactory(get(ExecutorFactory.class)); } @@ -441,6 +449,7 @@ protected SettingsProcessor createSettingsProcessor( ScriptHandlerFactory scriptHandlerFactory, Instantiator instantiator, ServiceRegistryFactory serviceRegistryFactory, + GradlePropertiesController gradlePropertiesController, GradleProperties gradleProperties, BuildOperationExecutor buildOperationExecutor, TextFileResourceLoader textFileResourceLoader @@ -448,15 +457,18 @@ protected SettingsProcessor createSettingsProcessor( return new BuildOperationSettingsProcessor( new RootBuildCacheControllerSettingsProcessor( new SettingsEvaluatedCallbackFiringSettingsProcessor( - new ScriptEvaluatingSettingsProcessor( - scriptPluginFactory, - new SettingsFactory( - instantiator, - serviceRegistryFactory, - scriptHandlerFactory + new PropertiesLoadingSettingsProcessor( + new ScriptEvaluatingSettingsProcessor( + scriptPluginFactory, + new SettingsFactory( + instantiator, + serviceRegistryFactory, + scriptHandlerFactory + ), + gradleProperties, + textFileResourceLoader ), - gradleProperties, - textFileResourceLoader + gradlePropertiesController ) ) ), diff --git a/subprojects/core/src/test/groovy/org/gradle/initialization/DefaultGradlePropertiesControllerTest.groovy b/subprojects/core/src/test/groovy/org/gradle/initialization/DefaultGradlePropertiesControllerTest.groovy new file mode 100644 index 000000000000..368b6464c8a4 --- /dev/null +++ b/subprojects/core/src/test/groovy/org/gradle/initialization/DefaultGradlePropertiesControllerTest.groovy @@ -0,0 +1,101 @@ +/* + * Copyright 2020 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.gradle.initialization + +import org.gradle.api.internal.properties.GradleProperties +import spock.lang.Specification +import spock.lang.Unroll + +class DefaultGradlePropertiesControllerTest extends Specification { + + @Unroll + def "attached GradleProperties #method fails before loading"() { + + given: + def propertiesLoader = Mock(IGradlePropertiesLoader) + def subject = new DefaultGradlePropertiesController(propertiesLoader) + def properties = subject.gradleProperties + 0 * propertiesLoader.loadGradleProperties(_) + + when: + switch (method) { + case "find": properties.find("anything"); break + case "mergeProperties": properties.mergeProperties([:]); break + default: assert false + } + + then: + thrown(IllegalStateException) + + where: + method << ["find", "mergeProperties"] + } + + def "attached GradleProperties methods succeed after loading"() { + + given: + def settingsDir = new File('.') + def propertiesLoader = Mock(IGradlePropertiesLoader) + def subject = new DefaultGradlePropertiesController(propertiesLoader) + def properties = subject.gradleProperties + def loadedProperties = Mock(GradleProperties) + 1 * propertiesLoader.loadGradleProperties(settingsDir) >> loadedProperties + 1 * loadedProperties.mergeProperties(_) >> [property: '42'] + 1 * loadedProperties.find(_) >> '42' + + when: + subject.loadGradlePropertiesFrom(settingsDir) + + then: + properties.find("property") == '42' + properties.mergeProperties([:]) == [property: '42'] + } + + def "loadGradlePropertiesFrom is idempotent"() { + + given: + // use a different File instance for each call to ensure it is compared by value + def currentDir = { new File('.') } + def propertiesLoader = Mock(IGradlePropertiesLoader) + def subject = new DefaultGradlePropertiesController(propertiesLoader) + def loadedProperties = Mock(GradleProperties) + + when: "calling the method multiple times with the same value" + subject.loadGradlePropertiesFrom(currentDir()) + subject.loadGradlePropertiesFrom(currentDir()) + + then: + 1 * propertiesLoader.loadGradleProperties(currentDir()) >> loadedProperties + } + + def "loadGradlePropertiesFrom fails when called with different argument"() { + + given: + def settingsDir = new File('a') + def propertiesLoader = Mock(IGradlePropertiesLoader) + def subject = new DefaultGradlePropertiesController(propertiesLoader) + def loadedProperties = Mock(GradleProperties) + 1 * propertiesLoader.loadGradleProperties(settingsDir) >> loadedProperties + + when: + subject.loadGradlePropertiesFrom(settingsDir) + subject.loadGradlePropertiesFrom(new File('b')) + + then: + thrown(IllegalStateException) + } +} diff --git a/subprojects/instant-execution/src/main/kotlin/org/gradle/instantexecution/DefaultInstantExecution.kt b/subprojects/instant-execution/src/main/kotlin/org/gradle/instantexecution/DefaultInstantExecution.kt index 6eef2cc2d539..90f8c19190f9 100644 --- a/subprojects/instant-execution/src/main/kotlin/org/gradle/instantexecution/DefaultInstantExecution.kt +++ b/subprojects/instant-execution/src/main/kotlin/org/gradle/instantexecution/DefaultInstantExecution.kt @@ -27,6 +27,7 @@ import org.gradle.api.provider.Provider import org.gradle.api.provider.ValueSource import org.gradle.api.provider.ValueSourceParameters import org.gradle.execution.plan.Node +import org.gradle.initialization.GradlePropertiesController import org.gradle.initialization.InstantExecution import org.gradle.instantexecution.extensions.uncheckedCast import org.gradle.instantexecution.extensions.unsafeLazy @@ -70,7 +71,8 @@ class DefaultInstantExecution internal constructor( private val scopeRegistryListener: InstantExecutionClassLoaderScopeRegistryListener, private val beanConstructors: BeanConstructors, private val valueSourceProviderFactory: ValueSourceProviderFactory, - private val virtualFileSystem: VirtualFileSystem + private val virtualFileSystem: VirtualFileSystem, + private val gradlePropertiesController: GradlePropertiesController ) : InstantExecution { interface Host { @@ -101,6 +103,12 @@ class DefaultInstantExecution internal constructor( false } else -> { + + // TODO - should load properties from settings file directory + gradlePropertiesController.loadGradlePropertiesFrom( + startParameter.rootDirectory + ) + val fingerprintChangedReason = checkFingerprint() when { fingerprintChangedReason != null -> { From e236364f556a013141a5800093ed98332867b3da Mon Sep 17 00:00:00 2001 From: "Rodrigo B. de Oliveira" Date: Mon, 2 Mar 2020 19:52:18 -0300 Subject: [PATCH 02/13] Ignore test for instant execution enablement via `gradle.properties` And mark it `@ToBeImplemented`. --- .../InstantExecutionEnablingIntegrationTest.groovy | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/subprojects/instant-execution/src/integTest/groovy/org/gradle/instantexecution/InstantExecutionEnablingIntegrationTest.groovy b/subprojects/instant-execution/src/integTest/groovy/org/gradle/instantexecution/InstantExecutionEnablingIntegrationTest.groovy index 60acd669a6d9..e008b21771dd 100644 --- a/subprojects/instant-execution/src/integTest/groovy/org/gradle/instantexecution/InstantExecutionEnablingIntegrationTest.groovy +++ b/subprojects/instant-execution/src/integTest/groovy/org/gradle/instantexecution/InstantExecutionEnablingIntegrationTest.groovy @@ -17,6 +17,8 @@ package org.gradle.instantexecution import org.gradle.integtests.fixtures.executer.AbstractGradleExecuter +import org.gradle.util.ToBeImplemented +import spock.lang.Ignore class InstantExecutionEnablingIntegrationTest extends AbstractInstantExecutionIntegrationTest { @@ -58,6 +60,8 @@ class InstantExecutionEnablingIntegrationTest extends AbstractInstantExecutionIn } } + @Ignore + @ToBeImplemented def "can enable instant execution from gradle.properties"() { given: From 735b834169d12476e403bb898940aef7c14c77bd Mon Sep 17 00:00:00 2001 From: "Rodrigo B. de Oliveira" Date: Tue, 3 Mar 2020 10:16:13 -0300 Subject: [PATCH 03/13] Move `gradle.properties` loading to immediately after the settings location becomes available --- .../initialization/DefaultSettingsLoader.java | 17 ++++++- .../DefaultSettingsLoaderFactory.java | 21 +++++++-- .../PropertiesLoadingSettingsProcessor.java | 47 ------------------- .../service/scopes/BuildScopeServices.java | 34 ++++++++------ .../DefaultSettingsLoaderTest.groovy | 4 +- 5 files changed, 56 insertions(+), 67 deletions(-) delete mode 100644 subprojects/core/src/main/java/org/gradle/initialization/PropertiesLoadingSettingsProcessor.java diff --git a/subprojects/core/src/main/java/org/gradle/initialization/DefaultSettingsLoader.java b/subprojects/core/src/main/java/org/gradle/initialization/DefaultSettingsLoader.java index 0f888c9dc106..d9e4c41f0546 100644 --- a/subprojects/core/src/main/java/org/gradle/initialization/DefaultSettingsLoader.java +++ b/subprojects/core/src/main/java/org/gradle/initialization/DefaultSettingsLoader.java @@ -36,16 +36,25 @@ public class DefaultSettingsLoader implements SettingsLoader { public static final String BUILD_SRC_PROJECT_PATH = ":" + SettingsInternal.BUILD_SRC; private SettingsProcessor settingsProcessor; private final BuildLayoutFactory buildLayoutFactory; + private final GradlePropertiesController gradlePropertiesController; - public DefaultSettingsLoader(SettingsProcessor settingsProcessor, BuildLayoutFactory buildLayoutFactory) { + public DefaultSettingsLoader( + SettingsProcessor settingsProcessor, + BuildLayoutFactory buildLayoutFactory, + GradlePropertiesController gradlePropertiesController + ) { this.settingsProcessor = settingsProcessor; this.buildLayoutFactory = buildLayoutFactory; + this.gradlePropertiesController = gradlePropertiesController; } @Override public SettingsInternal findAndLoadSettings(GradleInternal gradle) { StartParameter startParameter = gradle.getStartParameter(); + SettingsLocation settingsLocation = buildLayoutFactory.getLayoutFor(new BuildLayoutConfiguration(startParameter)); + loadGradlePropertiesFrom(settingsLocation); + SettingsInternal settings = findSettingsAndLoadIfAppropriate(gradle, startParameter, settingsLocation, gradle.getClassLoaderScope()); ProjectSpec spec = ProjectSpecs.forStartParameter(startParameter, settings); if (useEmptySettings(spec, settings, startParameter)) { @@ -56,6 +65,12 @@ public SettingsInternal findAndLoadSettings(GradleInternal gradle) { return settings; } + private void loadGradlePropertiesFrom(SettingsLocation settingsLocation) { + gradlePropertiesController.loadGradlePropertiesFrom( + settingsLocation.getSettingsDir() + ); + } + private boolean useEmptySettings(ProjectSpec spec, SettingsInternal loadedSettings, StartParameter startParameter) { // Never use empty settings when the settings were explicitly set if (startParameter.getSettingsFile() != null) { diff --git a/subprojects/core/src/main/java/org/gradle/initialization/DefaultSettingsLoaderFactory.java b/subprojects/core/src/main/java/org/gradle/initialization/DefaultSettingsLoaderFactory.java index addaa4fe53ad..0350278f3c3d 100644 --- a/subprojects/core/src/main/java/org/gradle/initialization/DefaultSettingsLoaderFactory.java +++ b/subprojects/core/src/main/java/org/gradle/initialization/DefaultSettingsLoaderFactory.java @@ -32,14 +32,24 @@ public class DefaultSettingsLoaderFactory implements SettingsLoaderFactory { private final PublicBuildPath publicBuildPath; private final Instantiator instantiator; private final BuildLayoutFactory buildLayoutFactory; + private GradlePropertiesController gradlePropertiesController; - public DefaultSettingsLoaderFactory(SettingsProcessor settingsProcessor, BuildStateRegistry buildRegistry, ProjectStateRegistry projectRegistry, PublicBuildPath publicBuildPath, Instantiator instantiator, BuildLayoutFactory buildLayoutFactory) { + public DefaultSettingsLoaderFactory( + SettingsProcessor settingsProcessor, + BuildStateRegistry buildRegistry, + ProjectStateRegistry projectRegistry, + PublicBuildPath publicBuildPath, + Instantiator instantiator, + BuildLayoutFactory buildLayoutFactory, + GradlePropertiesController gradlePropertiesController + ) { this.settingsProcessor = settingsProcessor; this.buildRegistry = buildRegistry; this.projectRegistry = projectRegistry; this.publicBuildPath = publicBuildPath; this.instantiator = instantiator; this.buildLayoutFactory = buildLayoutFactory; + this.gradlePropertiesController = gradlePropertiesController; } @Override @@ -68,7 +78,12 @@ public SettingsLoader forNestedBuild() { private SettingsLoader defaultSettingsLoader() { return new SettingsAttachingSettingsLoader( - new DefaultSettingsLoader(settingsProcessor, buildLayoutFactory), - projectRegistry); + new DefaultSettingsLoader( + settingsProcessor, + buildLayoutFactory, + gradlePropertiesController + ), + projectRegistry + ); } } diff --git a/subprojects/core/src/main/java/org/gradle/initialization/PropertiesLoadingSettingsProcessor.java b/subprojects/core/src/main/java/org/gradle/initialization/PropertiesLoadingSettingsProcessor.java deleted file mode 100644 index 675567f844b7..000000000000 --- a/subprojects/core/src/main/java/org/gradle/initialization/PropertiesLoadingSettingsProcessor.java +++ /dev/null @@ -1,47 +0,0 @@ -/* - * Copyright 2020 the original author or authors. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package org.gradle.initialization; - -import org.gradle.StartParameter; -import org.gradle.api.internal.GradleInternal; -import org.gradle.api.internal.SettingsInternal; -import org.gradle.api.internal.initialization.ClassLoaderScope; - -public class PropertiesLoadingSettingsProcessor implements SettingsProcessor { - - private final SettingsProcessor delegate; - private final GradlePropertiesController gradlePropertiesController; - - public PropertiesLoadingSettingsProcessor( - SettingsProcessor delegate, - GradlePropertiesController gradlePropertiesController - ) { - this.delegate = delegate; - this.gradlePropertiesController = gradlePropertiesController; - } - - @Override - public SettingsInternal process( - GradleInternal gradle, - SettingsLocation settingsLocation, - ClassLoaderScope buildRootClassLoaderScope, - StartParameter startParameter - ) { - gradlePropertiesController.loadGradlePropertiesFrom(settingsLocation.getSettingsDir()); - return delegate.process(gradle, settingsLocation, buildRootClassLoaderScope, startParameter); - } -} diff --git a/subprojects/core/src/main/java/org/gradle/internal/service/scopes/BuildScopeServices.java b/subprojects/core/src/main/java/org/gradle/internal/service/scopes/BuildScopeServices.java index 2ac307a395b5..08b016a4cac9 100644 --- a/subprojects/core/src/main/java/org/gradle/internal/service/scopes/BuildScopeServices.java +++ b/subprojects/core/src/main/java/org/gradle/internal/service/scopes/BuildScopeServices.java @@ -135,7 +135,6 @@ import org.gradle.initialization.ProjectAccessListener; import org.gradle.initialization.ProjectDescriptorRegistry; import org.gradle.initialization.ProjectPropertySettingBuildLoader; -import org.gradle.initialization.PropertiesLoadingSettingsProcessor; import org.gradle.initialization.RootBuildCacheControllerSettingsProcessor; import org.gradle.initialization.ScriptEvaluatingSettingsProcessor; import org.gradle.initialization.SettingsEvaluatedCallbackFiringSettingsProcessor; @@ -409,14 +408,23 @@ private DefaultScriptPluginFactory defaultScriptPluginFactory() { get(Deleter.class)); } - protected SettingsLoaderFactory createSettingsLoaderFactory(SettingsProcessor settingsProcessor, BuildStateRegistry buildRegistry, ProjectStateRegistry projectRegistry, PublicBuildPath publicBuildPath, Instantiator instantiator, BuildLayoutFactory buildLayoutFactory) { + protected SettingsLoaderFactory createSettingsLoaderFactory( + SettingsProcessor settingsProcessor, + BuildStateRegistry buildRegistry, + ProjectStateRegistry projectRegistry, + PublicBuildPath publicBuildPath, + Instantiator instantiator, + BuildLayoutFactory buildLayoutFactory, + GradlePropertiesController gradlePropertiesController + ) { return new DefaultSettingsLoaderFactory( settingsProcessor, buildRegistry, projectRegistry, publicBuildPath, instantiator, - buildLayoutFactory + buildLayoutFactory, + gradlePropertiesController ); } @@ -449,7 +457,6 @@ protected SettingsProcessor createSettingsProcessor( ScriptHandlerFactory scriptHandlerFactory, Instantiator instantiator, ServiceRegistryFactory serviceRegistryFactory, - GradlePropertiesController gradlePropertiesController, GradleProperties gradleProperties, BuildOperationExecutor buildOperationExecutor, TextFileResourceLoader textFileResourceLoader @@ -457,18 +464,15 @@ protected SettingsProcessor createSettingsProcessor( return new BuildOperationSettingsProcessor( new RootBuildCacheControllerSettingsProcessor( new SettingsEvaluatedCallbackFiringSettingsProcessor( - new PropertiesLoadingSettingsProcessor( - new ScriptEvaluatingSettingsProcessor( - scriptPluginFactory, - new SettingsFactory( - instantiator, - serviceRegistryFactory, - scriptHandlerFactory - ), - gradleProperties, - textFileResourceLoader + new ScriptEvaluatingSettingsProcessor( + scriptPluginFactory, + new SettingsFactory( + instantiator, + serviceRegistryFactory, + scriptHandlerFactory ), - gradlePropertiesController + gradleProperties, + textFileResourceLoader ) ) ), diff --git a/subprojects/core/src/test/groovy/org/gradle/initialization/DefaultSettingsLoaderTest.groovy b/subprojects/core/src/test/groovy/org/gradle/initialization/DefaultSettingsLoaderTest.groovy index 424d5cff81d7..abc97d26b2bd 100644 --- a/subprojects/core/src/test/groovy/org/gradle/initialization/DefaultSettingsLoaderTest.groovy +++ b/subprojects/core/src/test/groovy/org/gradle/initialization/DefaultSettingsLoaderTest.groovy @@ -39,7 +39,8 @@ class DefaultSettingsLoaderTest extends Specification { def startParameter = new StartParameterInternal() def classLoaderScope = Mock(ClassLoaderScope) def settingsProcessor = Mock(SettingsProcessor) - def settingsHandler = new DefaultSettingsLoader(settingsProcessor, buildLayoutFactory) + def gradlePropertiesController = Mock(GradlePropertiesController) + def settingsHandler = new DefaultSettingsLoader(settingsProcessor, buildLayoutFactory, gradlePropertiesController) void findAndLoadSettingsWithExistingSettings() { when: @@ -62,6 +63,7 @@ class DefaultSettingsLoaderTest extends Specification { 1 * settingsProcessor.process(gradle, buildLayout, classLoaderScope, startParameter) >> settings 1 * settings.settingsScript >> settingsScript 1 * settingsScript.displayName >> "foo" + 1 * gradlePropertiesController.loadGradlePropertiesFrom(buildLayout.settingsDir) then: settingsHandler.findAndLoadSettings(gradle).is(settings) From 8a4b24f615c2f932db0bc4af5af9cbaaedaf9132 Mon Sep 17 00:00:00 2001 From: "Rodrigo B. de Oliveira" Date: Tue, 3 Mar 2020 10:17:15 -0300 Subject: [PATCH 04/13] Ignore `system properties from gradle.properties are available to init scripts for buildSrc` As it cannot be safely implemented with the current init scripts semantics as init scripts can influence the settings file location thus influencing the location of the `gradle.properties` file. --- .../org/gradle/launcher/BuildEnvironmentIntegrationTest.groovy | 2 ++ 1 file changed, 2 insertions(+) diff --git a/subprojects/launcher/src/integTest/groovy/org/gradle/launcher/BuildEnvironmentIntegrationTest.groovy b/subprojects/launcher/src/integTest/groovy/org/gradle/launcher/BuildEnvironmentIntegrationTest.groovy index 545babfb1dfd..fa658825fdb5 100644 --- a/subprojects/launcher/src/integTest/groovy/org/gradle/launcher/BuildEnvironmentIntegrationTest.groovy +++ b/subprojects/launcher/src/integTest/groovy/org/gradle/launcher/BuildEnvironmentIntegrationTest.groovy @@ -16,6 +16,7 @@ package org.gradle.launcher +import groovy.transform.NotYetImplemented import org.gradle.integtests.fixtures.AbstractIntegrationSpec import org.gradle.integtests.fixtures.ToBeFixedForInstantExecution import org.gradle.integtests.fixtures.executer.ExecutionResult @@ -226,6 +227,7 @@ task check { } @Issue("https://github.com/gradle/gradle/issues/1001") + @NotYetImplemented def "system properties from gradle.properties are available to init scripts for buildSrc"() { given: executer.requireOwnGradleUserHomeDir() From 599bb3f8afcf1ca5a216a14fb61d6779d6fe2597 Mon Sep 17 00:00:00 2001 From: "Rodrigo B. de Oliveira" Date: Tue, 3 Mar 2020 11:37:18 -0300 Subject: [PATCH 05/13] Revert "Adjust `DistributionPropertiesLoaderIntegrationTest` to project properties being available to buildSrc" This reverts commit 1a708692 Signed-off-by: Rodrigo B. de Oliveira --- .../DistributionPropertiesLoaderIntegrationTest.groovy | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/subprojects/core/src/integTest/groovy/org/gradle/initialization/DistributionPropertiesLoaderIntegrationTest.groovy b/subprojects/core/src/integTest/groovy/org/gradle/initialization/DistributionPropertiesLoaderIntegrationTest.groovy index 3590ae79697e..c264dafdc8b5 100644 --- a/subprojects/core/src/integTest/groovy/org/gradle/initialization/DistributionPropertiesLoaderIntegrationTest.groovy +++ b/subprojects/core/src/integTest/groovy/org/gradle/initialization/DistributionPropertiesLoaderIntegrationTest.groovy @@ -61,11 +61,11 @@ class DistributionPropertiesLoaderIntegrationTest extends AbstractIntegrationSpe then: outputContains('system_property_available in buildSrc: true') outputContains('system_property_available in buildSrc: true') - outputContains('project_property_available in buildSrc: true') + outputContains('project_property_available in buildSrc: false') outputContains('system_property_available in included buildSrc: true') - outputContains('project_property_available in included buildSrc: true') + outputContains('project_property_available in included buildSrc: false') outputContains('system_property_available in included root: true') - outputContains('project_property_available in included root: true') + outputContains('project_property_available in included root: false') outputContains('system_property_available in root: true') outputContains('project_property_available in root: true') outputContains('system_property_available in settings.gradle: true') From 5072537fa1afadb507b7a8047433137af5aa5b6a Mon Sep 17 00:00:00 2001 From: "Rodrigo B. de Oliveira" Date: Tue, 3 Mar 2020 11:42:06 -0300 Subject: [PATCH 06/13] Augment `DistributionPropertiesLoaderIntegrationTest` Also test for the availability of Gradle properties in `settings.gradle`. --- ...istributionPropertiesLoaderIntegrationTest.groovy | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/subprojects/core/src/integTest/groovy/org/gradle/initialization/DistributionPropertiesLoaderIntegrationTest.groovy b/subprojects/core/src/integTest/groovy/org/gradle/initialization/DistributionPropertiesLoaderIntegrationTest.groovy index c264dafdc8b5..1e7b39c92923 100644 --- a/subprojects/core/src/integTest/groovy/org/gradle/initialization/DistributionPropertiesLoaderIntegrationTest.groovy +++ b/subprojects/core/src/integTest/groovy/org/gradle/initialization/DistributionPropertiesLoaderIntegrationTest.groovy @@ -29,6 +29,11 @@ class DistributionPropertiesLoaderIntegrationTest extends AbstractIntegrationSpe settingsFile << ''' includeBuild 'includedBuild' println("system_property_available in settings.gradle: ${System.getProperty('system_property_available', 'false')} ") + try { + println("project_property_available in settings.gradle: ${project_property_available} ") + } catch (MissingPropertyException e) { + println("project_property_available in settings.gradle: false ") + } ''' buildFile << ''' println("system_property_available in root: ${System.getProperty('system_property_available', 'false')} ") @@ -45,6 +50,11 @@ class DistributionPropertiesLoaderIntegrationTest extends AbstractIntegrationSpe ''' file('includedBuild/settings.gradle') << ''' println("system_property_available in included settings.gradle: ${System.getProperty('system_property_available', 'false')} ") + try { + println("project_property_available in included settings.gradle:${project_property_available} ") + } catch (MissingPropertyException e) { + println("project_property_available in included settings.gradle:false ") + } ''' file('includedBuild/buildSrc/build.gradle') << ''' println("system_property_available in included buildSrc: ${System.getProperty('system_property_available', 'false')} ") @@ -69,7 +79,9 @@ class DistributionPropertiesLoaderIntegrationTest extends AbstractIntegrationSpe outputContains('system_property_available in root: true') outputContains('project_property_available in root: true') outputContains('system_property_available in settings.gradle: true') + outputContains('project_property_available in settings.gradle: true') outputContains('system_property_available in included settings.gradle: true') + outputContains('project_property_available in included settings.gradle:false') cleanup: executer.withArguments("--stop", "--info").run() From 8383d614143515ac198e9030a7167fd80f932bf8 Mon Sep 17 00:00:00 2001 From: "Rodrigo B. de Oliveira" Date: Tue, 3 Mar 2020 11:44:12 -0300 Subject: [PATCH 07/13] Polish `ToolingApiPropertiesLoaderCrossVersionSpec` --- .../r60/ToolingApiPropertiesLoaderCrossVersionSpec.groovy | 4 ++-- .../r62/ToolingApiPropertiesLoaderCrossVersionSpec.groovy | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/subprojects/tooling-api/src/crossVersionTest/groovy/org/gradle/integtests/tooling/r60/ToolingApiPropertiesLoaderCrossVersionSpec.groovy b/subprojects/tooling-api/src/crossVersionTest/groovy/org/gradle/integtests/tooling/r60/ToolingApiPropertiesLoaderCrossVersionSpec.groovy index b4cbf4ad67ae..e0cff6888b0e 100644 --- a/subprojects/tooling-api/src/crossVersionTest/groovy/org/gradle/integtests/tooling/r60/ToolingApiPropertiesLoaderCrossVersionSpec.groovy +++ b/subprojects/tooling-api/src/crossVersionTest/groovy/org/gradle/integtests/tooling/r60/ToolingApiPropertiesLoaderCrossVersionSpec.groovy @@ -97,9 +97,9 @@ abstract class AbstractToolingApiPropertiesLoaderCrossVersionSpec extends Toolin output.contains('system_property_available in included settings.gradle: true') } + abstract boolean projectPropertyAvailableInBuildSrc(); + abstract boolean projectPropertyAvailableInIncludedRoot(); abstract boolean projectPropertyAvailableInIncludedBuildSrc(); - - abstract boolean projectPropertyAvailableInBuildSrc(); } diff --git a/subprojects/tooling-api/src/crossVersionTest/groovy/org/gradle/integtests/tooling/r62/ToolingApiPropertiesLoaderCrossVersionSpec.groovy b/subprojects/tooling-api/src/crossVersionTest/groovy/org/gradle/integtests/tooling/r62/ToolingApiPropertiesLoaderCrossVersionSpec.groovy index 118361eaecd6..b6aefdfa7eec 100644 --- a/subprojects/tooling-api/src/crossVersionTest/groovy/org/gradle/integtests/tooling/r62/ToolingApiPropertiesLoaderCrossVersionSpec.groovy +++ b/subprojects/tooling-api/src/crossVersionTest/groovy/org/gradle/integtests/tooling/r62/ToolingApiPropertiesLoaderCrossVersionSpec.groovy @@ -25,17 +25,17 @@ import org.gradle.integtests.tooling.r60.AbstractToolingApiPropertiesLoaderCross class ToolingApiPropertiesLoaderCrossVersionSpec extends AbstractToolingApiPropertiesLoaderCrossVersionSpec { @Override - boolean projectPropertyAvailableInIncludedRoot() { + boolean projectPropertyAvailableInBuildSrc() { true } @Override - boolean projectPropertyAvailableInIncludedBuildSrc() { + boolean projectPropertyAvailableInIncludedRoot() { true } @Override - boolean projectPropertyAvailableInBuildSrc() { + boolean projectPropertyAvailableInIncludedBuildSrc() { true } } From ea6cf14cf07dc12361aa342e62ea740e5a24aa07 Mon Sep 17 00:00:00 2001 From: "Rodrigo B. de Oliveira" Date: Tue, 3 Mar 2020 11:44:57 -0300 Subject: [PATCH 08/13] Branch `ToolingApiPropertiesLoaderCrossVersionSpec` for Gradle >= 6.2.2 Restoring the pre-6.2 behavior. --- ...ApiPropertiesLoaderCrossVersionSpec.groovy | 2 +- ...ApiPropertiesLoaderCrossVersionSpec.groovy | 42 +++++++++++++++++++ 2 files changed, 43 insertions(+), 1 deletion(-) create mode 100644 subprojects/tooling-api/src/crossVersionTest/groovy/org/gradle/integtests/tooling/r622/ToolingApiPropertiesLoaderCrossVersionSpec.groovy diff --git a/subprojects/tooling-api/src/crossVersionTest/groovy/org/gradle/integtests/tooling/r62/ToolingApiPropertiesLoaderCrossVersionSpec.groovy b/subprojects/tooling-api/src/crossVersionTest/groovy/org/gradle/integtests/tooling/r62/ToolingApiPropertiesLoaderCrossVersionSpec.groovy index b6aefdfa7eec..e81a99c04b00 100644 --- a/subprojects/tooling-api/src/crossVersionTest/groovy/org/gradle/integtests/tooling/r62/ToolingApiPropertiesLoaderCrossVersionSpec.groovy +++ b/subprojects/tooling-api/src/crossVersionTest/groovy/org/gradle/integtests/tooling/r62/ToolingApiPropertiesLoaderCrossVersionSpec.groovy @@ -21,7 +21,7 @@ import org.gradle.integtests.tooling.fixture.ToolingApiVersion import org.gradle.integtests.tooling.r60.AbstractToolingApiPropertiesLoaderCrossVersionSpec @ToolingApiVersion('>=3.0') -@TargetGradleVersion('>=6.2') +@TargetGradleVersion('>=6.2 <6.2.1') class ToolingApiPropertiesLoaderCrossVersionSpec extends AbstractToolingApiPropertiesLoaderCrossVersionSpec { @Override diff --git a/subprojects/tooling-api/src/crossVersionTest/groovy/org/gradle/integtests/tooling/r622/ToolingApiPropertiesLoaderCrossVersionSpec.groovy b/subprojects/tooling-api/src/crossVersionTest/groovy/org/gradle/integtests/tooling/r622/ToolingApiPropertiesLoaderCrossVersionSpec.groovy new file mode 100644 index 000000000000..8d054d67cf8c --- /dev/null +++ b/subprojects/tooling-api/src/crossVersionTest/groovy/org/gradle/integtests/tooling/r622/ToolingApiPropertiesLoaderCrossVersionSpec.groovy @@ -0,0 +1,42 @@ +/* + * Copyright 2019 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.gradle.integtests.tooling.r622 + +import org.gradle.integtests.tooling.fixture.TargetGradleVersion +import org.gradle.integtests.tooling.fixture.ToolingApiVersion +import org.gradle.integtests.tooling.r60.AbstractToolingApiPropertiesLoaderCrossVersionSpec + +@ToolingApiVersion('>=3.0') +@TargetGradleVersion('>=6.2.2') +class ToolingApiPropertiesLoaderCrossVersionSpec extends AbstractToolingApiPropertiesLoaderCrossVersionSpec { + + @Override + boolean projectPropertyAvailableInBuildSrc() { + false + } + + @Override + boolean projectPropertyAvailableInIncludedRoot() { + false + } + + @Override + boolean projectPropertyAvailableInIncludedBuildSrc() { + false + } +} + From f9108e96f6e57bef07e064c8a23a6c930890037c Mon Sep 17 00:00:00 2001 From: "Rodrigo B. de Oliveira" Date: Tue, 3 Mar 2020 12:10:03 -0300 Subject: [PATCH 09/13] Polish `DefaultGradlePropertiesController` --- .../initialization/DefaultGradlePropertiesController.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/subprojects/core/src/main/java/org/gradle/initialization/DefaultGradlePropertiesController.java b/subprojects/core/src/main/java/org/gradle/initialization/DefaultGradlePropertiesController.java index 79856430eaf4..2496cd12c34e 100644 --- a/subprojects/core/src/main/java/org/gradle/initialization/DefaultGradlePropertiesController.java +++ b/subprojects/core/src/main/java/org/gradle/initialization/DefaultGradlePropertiesController.java @@ -56,13 +56,13 @@ public Map mergeProperties(Map properties) { } private GradleProperties gradleProperties() { - return state.getGradleProperties(); + return state.gradleProperties(); } } private interface State { - GradleProperties getGradleProperties(); + GradleProperties gradleProperties(); State loadGradlePropertiesFrom(File settingsDir); } @@ -70,7 +70,7 @@ private interface State { private class NotLoaded implements State { @Override - public GradleProperties getGradleProperties() { + public GradleProperties gradleProperties() { throw new IllegalStateException("GradleProperties has not been loaded yet."); } @@ -94,7 +94,7 @@ public Loaded(GradleProperties gradleProperties, File propertiesDir) { } @Override - public GradleProperties getGradleProperties() { + public GradleProperties gradleProperties() { return gradleProperties; } From 7acb23536aa429e7389e331cccfbf168c0831639 Mon Sep 17 00:00:00 2001 From: "Rodrigo B. de Oliveira" Date: Tue, 3 Mar 2020 12:10:15 -0300 Subject: [PATCH 10/13] Polish `DefaultSettingsLoaderFactory` - Make field final --- .../org/gradle/initialization/DefaultSettingsLoaderFactory.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/subprojects/core/src/main/java/org/gradle/initialization/DefaultSettingsLoaderFactory.java b/subprojects/core/src/main/java/org/gradle/initialization/DefaultSettingsLoaderFactory.java index 0350278f3c3d..3f4af0674a32 100644 --- a/subprojects/core/src/main/java/org/gradle/initialization/DefaultSettingsLoaderFactory.java +++ b/subprojects/core/src/main/java/org/gradle/initialization/DefaultSettingsLoaderFactory.java @@ -32,7 +32,7 @@ public class DefaultSettingsLoaderFactory implements SettingsLoaderFactory { private final PublicBuildPath publicBuildPath; private final Instantiator instantiator; private final BuildLayoutFactory buildLayoutFactory; - private GradlePropertiesController gradlePropertiesController; + private final GradlePropertiesController gradlePropertiesController; public DefaultSettingsLoaderFactory( SettingsProcessor settingsProcessor, From 9e1ff52bbb5eed1652b565559e536031607cdcd7 Mon Sep 17 00:00:00 2001 From: "Rodrigo B. de Oliveira" Date: Tue, 3 Mar 2020 15:06:27 -0300 Subject: [PATCH 11/13] Update upgrading guide note about Gradle properties no longer leaking into buildSrc and included builds --- .../src/docs/userguide/migration/upgrading_version_6.adoc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/subprojects/docs/src/docs/userguide/migration/upgrading_version_6.adoc b/subprojects/docs/src/docs/userguide/migration/upgrading_version_6.adoc index a2c324bbb04d..339831290429 100644 --- a/subprojects/docs/src/docs/userguide/migration/upgrading_version_6.adoc +++ b/subprojects/docs/src/docs/userguide/migration/upgrading_version_6.adoc @@ -46,13 +46,13 @@ For example, when the library does not support the required Java version. The practical effect is that now all <> have to be declared as such. Before, platform dependencies also worked, accidentally, when the `platform()` keyword was omitted for local platforms or platforms published with Gradle Module Metadata. -==== Properties from project root `gradle.properties` are shared with `buildSrc` and included builds +==== Properties from project root `gradle.properties` leaking into `buildSrc` and included builds -Before Gradle 6.2, Gradle and project properties set in the `gradle.properties` file in your project root directory were not shared with the `buildSrc` build or included builds. +There was a regression in Gradle 6.2 and Gradle 6.2.1 that caused Gradle properties set in the project root `gradle.properties` file to leak into the `buildSrc` build and any builds included by the root. -In Gradle 6.2, Gradle and project properties set in the `gradle.properties` file are shared with the `buildSrc` build and any builds included by the root. +This could cause your build to start failing if the `buildSrc` build or an included build suddenly found an unexpected or incompatible value for a property coming from the project root `gradle.properties` file. -This might cause your build to start failing if the `buildSrc` build or an included build suddenly finds an unexpected or incompatible value for a property coming from the project root `gradle.properties` file. If it does, you can fix it by setting a compatible value to the offending property in a `gradle.properties` file in the failing project directory, as property values set there will take precedence over property values coming from the project root. +The regression has been fixed in Gradle 6.2.2. === Deprecations From 240051fab40210b3ba173edd5c388e4aaff58389 Mon Sep 17 00:00:00 2001 From: "Rodrigo B. de Oliveira" Date: Tue, 3 Mar 2020 18:36:51 -0300 Subject: [PATCH 12/13] Prove `gradle.properties` precedence is honored for nested builds --- ...ositeBuildPropertiesIntegrationTest.groovy | 38 +++++++++++-------- 1 file changed, 22 insertions(+), 16 deletions(-) diff --git a/subprojects/composite-builds/src/integTest/groovy/org/gradle/integtests/composite/CompositeBuildPropertiesIntegrationTest.groovy b/subprojects/composite-builds/src/integTest/groovy/org/gradle/integtests/composite/CompositeBuildPropertiesIntegrationTest.groovy index 2fe408f77374..382472b07d75 100644 --- a/subprojects/composite-builds/src/integTest/groovy/org/gradle/integtests/composite/CompositeBuildPropertiesIntegrationTest.groovy +++ b/subprojects/composite-builds/src/integTest/groovy/org/gradle/integtests/composite/CompositeBuildPropertiesIntegrationTest.groovy @@ -25,28 +25,32 @@ class CompositeBuildPropertiesIntegrationTest extends AbstractIntegrationSpec { def "included build properties take precedence over root build properties"() { given: - createDir("included") { - file("gradle.properties") << """ - theProperty=included - """ - file("settings.gradle") << """ - println("included settings script: " + theProperty) - """ - file("build.gradle") << """ - println("included build script: " + theProperty) - """ + def createBuild = { String buildName, String dir -> + createDir(dir) { + file("gradle.properties") << """ + theProperty=$buildName + """ + file("settings.gradle") << """ + println("$buildName settings script: " + theProperty) + """ + file("build.gradle") << """ + println("$buildName build script: " + theProperty) + """ + } } - file("gradle.properties") << """ - theProperty=root - """ + + createBuild "root", "." settingsFile << """ includeBuild 'included' - println("root settings script: " + theProperty) """ - buildFile << """ - println("root build script: " + theProperty) + + createBuild "included", "included" + file("included/settings.gradle") << """ + includeBuild '../nested' """ + createBuild "nested", "nested" + when: run('help') @@ -55,5 +59,7 @@ class CompositeBuildPropertiesIntegrationTest extends AbstractIntegrationSpec { outputContains("root build script: root") outputContains("included settings script: included") outputContains("included build script: included") + outputContains("nested settings script: nested") + outputContains("nested build script: nested") } } From d4eff18e7e5df1f66f1d093238b192a447cf46a4 Mon Sep 17 00:00:00 2001 From: "Rodrigo B. de Oliveira" Date: Tue, 3 Mar 2020 22:45:21 -0300 Subject: [PATCH 13/13] Adjust `ToolingApiPropertiesLoaderCrossVersionSpec` version range The broken behavior is present in 6.2 and 6.2.1. --- .../r62/ToolingApiPropertiesLoaderCrossVersionSpec.groovy | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/subprojects/tooling-api/src/crossVersionTest/groovy/org/gradle/integtests/tooling/r62/ToolingApiPropertiesLoaderCrossVersionSpec.groovy b/subprojects/tooling-api/src/crossVersionTest/groovy/org/gradle/integtests/tooling/r62/ToolingApiPropertiesLoaderCrossVersionSpec.groovy index e81a99c04b00..ea5ce487b3a2 100644 --- a/subprojects/tooling-api/src/crossVersionTest/groovy/org/gradle/integtests/tooling/r62/ToolingApiPropertiesLoaderCrossVersionSpec.groovy +++ b/subprojects/tooling-api/src/crossVersionTest/groovy/org/gradle/integtests/tooling/r62/ToolingApiPropertiesLoaderCrossVersionSpec.groovy @@ -21,7 +21,7 @@ import org.gradle.integtests.tooling.fixture.ToolingApiVersion import org.gradle.integtests.tooling.r60.AbstractToolingApiPropertiesLoaderCrossVersionSpec @ToolingApiVersion('>=3.0') -@TargetGradleVersion('>=6.2 <6.2.1') +@TargetGradleVersion('>=6.2 <6.2.2') class ToolingApiPropertiesLoaderCrossVersionSpec extends AbstractToolingApiPropertiesLoaderCrossVersionSpec { @Override