On 24/07/2011, at 7:04 PM, Luke Daley wrote: > > On 23/07/2011, at 10:38 AM, Adam Murdoch wrote: > >> >> On 23/07/2011, at 2:53 AM, [email protected] wrote: >> >>> Branch: refs/heads/master >>> Home: https://github.com/gradle/gradle >>> >>> Commit: 94f497b6a90e0a3dc1c54364a450378405d42471 >>> >>> https://github.com/gradle/gradle/commit/94f497b6a90e0a3dc1c54364a450378405d42471 >>> Author: Luke Daley <[email protected]> >>> Date: 2011-07-22 (Fri, 22 Jul 2011) >>> >>> Changed paths: >>> M >>> subprojects/core/src/main/groovy/org/gradle/api/artifacts/Configuration.java >>> M >>> subprojects/core/src/main/groovy/org/gradle/api/internal/artifacts/configurations/DefaultConfiguration.java >>> M subprojects/core/src/main/groovy/org/gradle/util/WrapUtil.java >>> M >>> subprojects/core/src/test/groovy/org/gradle/api/internal/artifacts/configurations/DefaultConfigurationSpec.groovy >>> M >>> subprojects/core/src/test/groovy/org/gradle/api/internal/artifacts/configurations/DefaultConfigurationTest.java >>> M >>> subprojects/docs/src/samples/userguideOutput/configurationHandlingDependencies.out >>> M >>> subprojects/signing/src/main/groovy/org/gradle/plugins/signing/Sign.groovy >>> M >>> subprojects/signing/src/main/groovy/org/gradle/plugins/signing/SigningSettings.groovy >>> >>> Log Message: >>> ----------- >>> Built on usage of DomainObjectSet with Configuration to simplify it's API >>> (and keep the dependency/artifact collections live). >> >> Cool. I think we can tidy up Configuation a bit more: >> >> * Deprecate getDependencies(type), replace with >> getDependencies().withType(type) >> * Deprecate getAllDependencies(type), replace with >> getAllDependencies().withType(type) > > There's a problem with the following two: > >> * Deprecate addDependency(dep), replace with getDependencies().add(dep) >> * Deprecate addArtifact(art) and removeArtifact(art), replace with >> getArtifacts().add(art) and remove(art) > > We need to prevent modifications to dependencies and artifacts if the > configuration is unresolved. This is easy enough to do… > > Action<Object> preventChangesWhenNotUnresolvedAction = new > Action<Object>() { > public void execute(Object o) { > throwExceptionIfNotInUnresolvedState(); } > }; > > dependencies.whenObjectAdded(preventChangesWhenNotUnresolvedAction); > dependencies.whenObjectRemoved(preventChangesWhenNotUnresolvedAction); > artifacts.whenObjectAdded(preventChangesWhenNotUnresolvedAction); > artifacts.whenObjectRemoved(preventChangesWhenNotUnresolvedAction); > > > But the exception gets wrapped in a ListenerNotificationException. > > Any ideas? We could maybe use a sentinel exception type that wraps the real > exception we want to propagate. > > Or, should we live with the ListenerNotificationException wrapping?
The whenObjectAdded()/whenObjectRemoved() methods are really intended for notification after the fact, not validation before the fact. I'd be tempted to add beforeObjectAdded()/beforeObjectRemoved() notifications which can be used for validation, perhaps not on the public interface, but just as methods on DefaultDomainObjectCollection. Just btw, even if we fix this particular problem, the validation on configuration falls a bit short: * We don't check for changes to the dependency objects. For example, I can point a project dependency at a different configuration, or change the exclusions for an external dependency. * We don't check for changes to the target configuration of project dependencies. Or their transitive project dependencies. * We don't check for changes to inherited configurations. I wonder if we should instead use a caching approach, where we allow any changes to configuration, but cache and reuse the resolved dependency if the unresolved graph of dependencies has not changed. There are 2 benefits to this approach: * We have more options to reuse the cached result. For example, if my project does not declare any runtime dependencies, I can reuse the result from resolving the compile configuration. We can also reuse the result across builds, if we're running in the daemon. * We don't have to try to validate all the possible way that the graph can be constructed. We just need to use the result. This gives us some flexibility to add some more interesting ways to construct the graph (in particular, third party extensions). Having said this, I don't think we should try to fix this now, we should just replicate the existing behaviour. -- Adam Murdoch Gradle Co-founder http://www.gradle.org VP of Engineering, Gradleware Inc. - Gradle Training, Support, Consulting http://www.gradleware.com
