Copilot commented on code in PR #16566: URL: https://github.com/apache/grails-core/pull/16566#discussion_r4232278625
########## build-logic/plugins/src/main/groovy/org/apache/grails/buildsrc/DependencyCachePolicy.groovy: ########## @@ -0,0 +1,41 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You 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 + * + * https://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.apache.grails.buildsrc + +import groovy.transform.CompileStatic +import org.gradle.api.artifacts.ResolutionStrategy + +/** + * Fixed releases are not governed by these knobs. Dynamic listings are release metadata, + * so they stay at Gradle's 24 hour default. Changing modules (snapshots) are polled + * immediately on ordinary CI and within 10 minutes locally. A reproducible build must + * not re-resolve them, even when CI is also set. + */ +@CompileStatic +class DependencyCachePolicy { + + static void apply(ResolutionStrategy strategy, boolean ci, boolean reproducible) { + strategy.cacheDynamicVersionsFor(24, 'hours') + if (reproducible) { + strategy.cacheChangingModulesFor(24, 'hours') + } else if (ci) { + strategy.cacheChangingModulesFor(0, 'hours') + } else { + strategy.cacheChangingModulesFor(10, 'minutes') + } + } Review Comment: The class-level comment says a reproducible build “must not re-resolve” changing modules, but `cacheChangingModulesFor(24, 'hours')` will re-check snapshots once the cache is older than 24 hours. To match the stated behavior and avoid non-reproducible snapshot drift, set this to a much longer duration (effectively ‘never’ for a build environment) rather than 24 hours. ########## gradle/plugin-repositories.gradle: ########## @@ -21,7 +21,11 @@ ext.configurePluginRepositories = { RepositoryHandler repositories -> if (System.getenv('GRAILS_INCLUDE_MAVEN_LOCAL')) { repositories.mavenLocal() } - repositories.mavenCentral() + repositories.mavenCentral { + mavenContent { + releasesOnly() + } + } repositories.gradlePluginPortal() Review Comment: Maven Central is now restricted to `releasesOnly()`, but the Plugin Portal is still unfiltered here. That can reintroduce snapshot/dynamic metadata lookups (and extra upstream traffic) for plugin-portal-hosted artifacts consumed as libraries. Consider adding a `mavenContent { releasesOnly() }` block to `repositories.gradlePluginPortal()` here to align with the repository-pressure goal described in the PR. ########## build-logic/plugins/src/test/groovy/org/apache/grails/buildsrc/GrailsRepoSettingsPluginSpec.groovy: ########## @@ -0,0 +1,63 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You 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 + * + * https://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.apache.grails.buildsrc + +import org.gradle.testkit.runner.GradleRunner +import spock.lang.Specification +import spock.lang.TempDir + +class GrailsRepoSettingsPluginSpec extends Specification { + + @TempDir + File projectDir + + def "dependency repositories prefer Central and do not fall back to repo.grails.org"() { + given: + new File(projectDir, 'settings.gradle').text = ''' + plugins { + id 'org.apache.grails.buildsrc.repo' + } + + def lines = [] + dependencyResolutionManagement.repositories.each { repo -> + lines << repo.url.toString() + } + new File(settingsDir, 'repos.txt').text = lines.join(System.lineSeparator()) + ''' + new File(projectDir, 'build.gradle').text = '' + + when: + GradleRunner.create() + .withProjectDir(projectDir) + .withPluginClasspath() + .withArguments('--offline', 'help') + .withEnvironment(System.getenv().findAll { key, value -> key != 'GRAILS_INCLUDE_MAVEN_LOCAL' }) + .build() + + then: + List<String> urls = new File(projectDir, 'repos.txt').text.readLines() + urls == [ + 'https://repo.maven.apache.org/maven2/', + 'https://repo.gradle.org/gradle/libs-releases', + 'https://plugins.gradle.org/m2', + 'https://repository.apache.org/content/groups/snapshots', + 'https://central.sonatype.com/repository/maven-snapshots', + 'https://repository.apache.org/content/groups/staging', + ] Review Comment: This assertion is brittle because Gradle may render repository URLs with/without trailing slashes depending on Gradle version and repository type (e.g., Central often appears both as `.../maven2` and `.../maven2/`). To reduce flakiness, normalize URLs before comparison (e.g., strip a single trailing `/`) or assert on stable components (host/path) while still verifying order and absence of `repo.grails.org`. ########## build-logic/plugins/src/main/groovy/org/apache/grails/buildsrc/GrailsRepoSettingsPlugin.groovy: ########## @@ -32,7 +32,11 @@ class GrailsRepoSettingsPlugin implements Plugin<Settings> { if (System.getenv('GRAILS_INCLUDE_MAVEN_LOCAL')) { repo.mavenLocal() } - repo.mavenCentral() + repo.mavenCentral { + mavenContent { + it.releasesOnly() + } + } repo.gradlePluginPortal() Review Comment: In the `pluginManagement` repository block, `gradlePluginPortal()` is still unfiltered, while the later `dependencyResolutionManagement` block applies `releasesOnly()`. For consistency and to avoid unnecessary metadata checks, apply `mavenContent { releasesOnly() }` to the plugin portal in this section as well. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
