Copilot commented on code in PR #3964:
URL: https://github.com/apache/solr/pull/3964#discussion_r2628839665
##########
solr/docker/build.gradle:
##########
@@ -158,32 +186,64 @@ task dockerBuild() {
// Ensure that the docker image is rebuilt on build-arg changes or changes
in the docker context
inputs.properties([
- baseDockerImage: baseDockerImage
+ baseDockerImage: baseDockerImage,
+ dockerImageArch: dockerImageArch
])
var solrTgzConfiguration = isImageSlim() ? configurations.solrSlimTgz :
configurations.solrFullTgz
inputs.files(solrTgzConfiguration)
inputs.property("isSlimImage", isImageSlim())
+ inputs.property("isMultiArch", isMultiArch())
dependsOn(solrTgzConfiguration)
doLast {
- exec {
- standardInput = solrTgzConfiguration.singleFile.newDataInputStream()
- commandLine "docker", "build",
- "-f", "solr-${ -> project.version
}${dockerImageDistSuffix}/docker/Dockerfile",
- "--iidfile", imageIdFile,
- "--build-arg", "BASE_IMAGE=${ ->
inputs.properties.baseDockerImage}",
- "-"
+ def archValue = inputs.properties.dockerImageArch
+ def useSingleArch = !archValue.isEmpty() && !archValue.contains(',')
+
+ // Multi-arch builds are not supported by dockerBuild, only by dockerPush
+ if (!archValue.isEmpty() && archValue.contains(',')) {
+ throw new GradleException("Multi-architecture builds (SOLR_DOCKER_ARCH
with multiple platforms) are not supported by dockerBuild.\n" +
+ "Please use 'dockerPush' instead, which will build and push the
multi-architecture image in a single step.\n" +
+ "Example: SOLR_DOCKER_ARCH=linux/arm64,linux/amd64 gradlew
dockerPush")
Review Comment:
The error message uses 'gradlew' without './' prefix, which is inconsistent
with the documentation in docker.txt that uses './gradlew'. Consider using
'./gradlew' for consistency with the rest of the documentation.
```suggestion
"Example: SOLR_DOCKER_ARCH=linux/arm64,linux/amd64 ./gradlew
dockerPush")
```
##########
solr/docker/build.gradle:
##########
@@ -234,19 +294,62 @@ task dockerPush(dependsOn: tasks.dockerTag) {
// Ensure that the docker image is re-pushed if the image ID or tag changes
inputs.properties([
dockerImageName: dockerImageName,
+ dockerImageArch: dockerImageArch
])
inputs.file(imageIdFile)
// We don't want to push a docker image unless the tests have passed
mustRunAfter tasks.testDocker
doLast {
- exec {
- commandLine "docker", "push", dockerImageName
- }
+ def archValue = inputs.properties.dockerImageArch
+ def useMultiArch = !archValue.isEmpty()
+
+ if (useMultiArch && isMultiArch()) {
Review Comment:
The condition `useMultiArch && isMultiArch()` is redundant since
`useMultiArch` is set to `!archValue.isEmpty()` and `isMultiArch()` checks both
`!isEmpty()` and `contains(',')`. This means the else branch at line 346 will
execute for single-arch builds with an explicit platform (e.g., "linux/arm64"),
which will attempt a standard `docker push` instead of using the buildx-built
image. This breaks the single-arch explicit platform workflow that was built in
dockerBuild.
```suggestion
if (useMultiArch) {
```
##########
solr/docker/build.gradle:
##########
@@ -234,19 +294,62 @@ task dockerPush(dependsOn: tasks.dockerTag) {
// Ensure that the docker image is re-pushed if the image ID or tag changes
inputs.properties([
dockerImageName: dockerImageName,
+ dockerImageArch: dockerImageArch
])
inputs.file(imageIdFile)
// We don't want to push a docker image unless the tests have passed
mustRunAfter tasks.testDocker
doLast {
- exec {
- commandLine "docker", "push", dockerImageName
- }
+ def archValue = inputs.properties.dockerImageArch
+ def useMultiArch = !archValue.isEmpty()
+
+ if (useMultiArch && isMultiArch()) {
+ // Multi-arch push requires buildx and rebuilding with --push
+
+ // Get builder info and validate it exists
+ def builderInfo = getBuildxBuilderInfo()
+
+ // Verify the builder supports the requested platforms (fail fast)
+ def requestedPlatforms = archValue.split(',').collect { it.trim() }
+ def missingPlatforms = []
+ requestedPlatforms.each { platform ->
+ // Check for exact platform match in the builder info
+ // Match pattern like "linux/amd64" or "linux/amd64," or "linux/amd64
" to avoid partial matches
+ if (!builderInfo.matches("(?s).*\\b" +
java.util.regex.Pattern.quote(platform) + "\\b.*")) {
+ missingPlatforms.add(platform)
+ }
+ }
- // Print information on the image after it has been created
- project.logger.lifecycle("Solr Docker Image Pushed: \t$dockerImageName")
+ if (!missingPlatforms.isEmpty()) {
+ throw new GradleException("Current builder does not support all
requested platforms: ${missingPlatforms}\n" +
+ "Supported platforms:
${builderInfo.findAll(/linux\/\w+(?:\/\w+)?/).join(', ')}\n" +
Review Comment:
The regex pattern `/linux\/\w+(?:\/\w+)?/` will only match simple platform
names and may not capture all supported platforms from the builder info. The
`findAll()` method is called on a String, which doesn't have this method - it
should be called on a Matcher. Consider parsing the builder info output more
reliably to extract supported platforms.
```suggestion
// Extract supported linux/* platforms from the builder info for a
helpful error message
def supportedPlatforms = (builderInfo =~
/linux\/[0-9A-Za-z_.-]+(?:\/[0-9A-Za-z_.-]+)*/).collect { it[0] }.unique()
throw new GradleException("Current builder does not support all
requested platforms: ${missingPlatforms}\n" +
"Supported platforms: ${supportedPlatforms.join(', ')}\n" +
```
##########
solr/docker/build.gradle:
##########
@@ -34,6 +34,8 @@ def dockerImageRepo = "${ ->
propertyOrEnvOrDefault("solr.docker.imageRepo", "SO
def dockerImageTag = "${ -> propertyOrEnvOrDefault("solr.docker.imageTag",
"SOLR_DOCKER_IMAGE_TAG", project.version + dockerImageDistSuffix) }"
def dockerImageName = "${ -> propertyOrEnvOrDefault("solr.docker.imageName",
"SOLR_DOCKER_IMAGE_NAME",
"${dockerImageRepo}:${dockerImageTag}${dockerImageTagSuffix}") }"
def baseDockerImage = "${ -> propertyOrEnvOrDefault("solr.docker.baseImage",
"SOLR_DOCKER_BASE_IMAGE", 'eclipse-temurin:' + javaVersion + '-jre-noble') }"
+def dockerImageArch = "${ -> propertyOrEnvOrDefault("solr.docker.arch",
"SOLR_DOCKER_ARCH", '') }"
+def isMultiArch = { -> !dockerImageArch.isEmpty() &&
dockerImageArch.contains(',') }
Review Comment:
The variable name 'dockerImageArch' is potentially misleading as it
represents platforms (e.g., 'linux/amd64') rather than just architectures
(e.g., 'amd64'). Consider renaming to 'dockerImagePlatform' or
'dockerImagePlatforms' to better reflect that it contains platform identifiers
including the OS.
```suggestion
def dockerImagePlatforms = "${ -> propertyOrEnvOrDefault("solr.docker.arch",
"SOLR_DOCKER_ARCH", '') }"
// Backward-compatible alias: dockerImageArch historically referred to what
is now called dockerImagePlatforms.
def dockerImageArch = dockerImagePlatforms
def isMultiArch = { -> !dockerImagePlatforms.isEmpty() &&
dockerImagePlatforms.contains(',') }
```
##########
solr/docker/build.gradle:
##########
@@ -234,19 +294,62 @@ task dockerPush(dependsOn: tasks.dockerTag) {
// Ensure that the docker image is re-pushed if the image ID or tag changes
inputs.properties([
dockerImageName: dockerImageName,
+ dockerImageArch: dockerImageArch
])
inputs.file(imageIdFile)
// We don't want to push a docker image unless the tests have passed
mustRunAfter tasks.testDocker
doLast {
- exec {
- commandLine "docker", "push", dockerImageName
- }
+ def archValue = inputs.properties.dockerImageArch
+ def useMultiArch = !archValue.isEmpty()
+
+ if (useMultiArch && isMultiArch()) {
+ // Multi-arch push requires buildx and rebuilding with --push
+
+ // Get builder info and validate it exists
+ def builderInfo = getBuildxBuilderInfo()
+
+ // Verify the builder supports the requested platforms (fail fast)
+ def requestedPlatforms = archValue.split(',').collect { it.trim() }
+ def missingPlatforms = []
+ requestedPlatforms.each { platform ->
+ // Check for exact platform match in the builder info
+ // Match pattern like "linux/amd64" or "linux/amd64," or "linux/amd64
" to avoid partial matches
+ if (!builderInfo.matches("(?s).*\\b" +
java.util.regex.Pattern.quote(platform) + "\\b.*")) {
Review Comment:
The word boundary matcher `\b` may not work correctly with platform strings
containing forward slashes. For example, `\b` doesn't match around `/`
characters, so this pattern could miss valid platforms or incorrectly match
partial strings. Consider using a more explicit pattern that checks for the
platform string followed by a comma, newline, or end of string.
```suggestion
// Match pattern like "linux/amd64" or "linux/amd64," or
"linux/amd64 " to avoid partial matches,
// using explicit separators (comma/whitespace/start/end) instead of
word boundaries, since
// platform strings contain '/' which does not work well with \b.
if (!builderInfo.matches("(?s).*(^|[\\s,])" +
java.util.regex.Pattern.quote(platform) + "($|[\\s,]).*")) {
```
##########
dev-docs/gradle-help/docker.txt:
##########
@@ -61,6 +75,38 @@ Docker Image Name: (Use this to explicitly set a whole image
name. If given, the
EnvVar: SOLR_DOCKER_IMAGE_NAME
Gradle Property: -Psolr.docker.imageName
+Multi-Architecture Builds
+-------
+
+To build a multi-architecture Docker image, use the SOLR_DOCKER_ARCH
environment variable or Gradle property.
+
+IMPORTANT: Multi-architecture builds (with multiple platforms) are ONLY
supported by dockerPush, not dockerBuild.
+This is because Docker cannot load multi-platform images into the local daemon.
+
+Prerequisites:
+ - Docker Buildx must be installed (included with Docker Desktop and recent
Docker Engine versions)
+ - A buildx builder must be configured. If you don't have one, create it:
+ docker buildx create --name solr-builder --use
+
+Build and push for multiple architectures:
+ SOLR_DOCKER_ARCH=linux/arm64,linux/amd64 ./gradlew dockerPush
+
+Using Gradle property:
+ ./gradlew dockerPush -Psolr.docker.arch=linux/arm64,linux/amd64
+
+The dockerPush task will build and push the multi-architecture image in a
single step using Docker Buildx.
+The task will validate that a suitable builder exists and that it supports all
requested platforms before building.
+
+Single-Architecture Builds with Explicit Platform
+--------------------------------------------------
+
+You can build for a specific architecture and load it into your local Docker
daemon:
+
+Build for a specific architecture:
+ SOLR_DOCKER_ARCH=linux/arm64 gradlew dockerBuild
Review Comment:
The command uses 'gradlew' without './' prefix, which is inconsistent with
line 92 which uses './gradlew'. Consider using './gradlew' for consistency.
##########
solr/docker/build.gradle:
##########
@@ -158,32 +186,64 @@ task dockerBuild() {
// Ensure that the docker image is rebuilt on build-arg changes or changes
in the docker context
inputs.properties([
- baseDockerImage: baseDockerImage
+ baseDockerImage: baseDockerImage,
+ dockerImageArch: dockerImageArch
])
var solrTgzConfiguration = isImageSlim() ? configurations.solrSlimTgz :
configurations.solrFullTgz
inputs.files(solrTgzConfiguration)
inputs.property("isSlimImage", isImageSlim())
+ inputs.property("isMultiArch", isMultiArch())
dependsOn(solrTgzConfiguration)
doLast {
- exec {
- standardInput = solrTgzConfiguration.singleFile.newDataInputStream()
- commandLine "docker", "build",
- "-f", "solr-${ -> project.version
}${dockerImageDistSuffix}/docker/Dockerfile",
- "--iidfile", imageIdFile,
- "--build-arg", "BASE_IMAGE=${ ->
inputs.properties.baseDockerImage}",
- "-"
+ def archValue = inputs.properties.dockerImageArch
+ def useSingleArch = !archValue.isEmpty() && !archValue.contains(',')
+
+ // Multi-arch builds are not supported by dockerBuild, only by dockerPush
+ if (!archValue.isEmpty() && archValue.contains(',')) {
Review Comment:
The variable 'useSingleArch' is defined here but the same logic is
duplicated on line 203 in the if condition. Consider reusing the
'useSingleArch' variable or the 'isMultiArch()' function to avoid code
duplication and potential inconsistencies.
```suggestion
if (!archValue.isEmpty() && !useSingleArch) {
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
