Copilot commented on code in PR #4227: URL: https://github.com/apache/solr/pull/4227#discussion_r3279730078
########## test-external-client/src/test/java/DependencySmokeTest.java: ########## @@ -0,0 +1,42 @@ +/* + * 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 + * + * 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. + */ + +import java.io.IOException; +import java.nio.file.Path; +import org.apache.solr.SolrTestCase; +import org.apache.solr.client.solrj.SolrServerException; +import org.apache.solr.util.EmbeddedSolrServerTestRule; +import org.apache.solr.util.SolrClientTestRule; +import org.junit.ClassRule; +import org.junit.Test; + +public class DependencySmokeTest extends SolrTestCase { + + @ClassRule + public static SolrClientTestRule solrRule = new EmbeddedSolrServerTestRule(); + + @Test + public void testSolrjAndTestFrameworkAreUsable() throws SolrServerException, IOException { + solrRule.startSolr(); + // TODO solr-test-framework.jar or maybe solr-core.jar ought to include the _default configSet + // and maybe a cloud-minimal configset. Once it does, then let's see if we can enhance + // withConfigSet to handle JAR Paths, and add a static getFile perhaps to make it easy. + Path configSet = Path.of("../solr/server/solr/configsets/_default/").toAbsolutePath(); + solrRule.newCollection().withConfigSet(configSet).create(); + solrRule.getSolrClient().ping(); Review Comment: This test hard-codes a configset path outside the test-external-client project (../solr/server/solr/configsets/_default). That makes the project non-standalone and will fail in the Docker-based Maven run (smokeTestRelease mounts only test-external-client at /project, so ../solr/... doesn’t exist). Consider vendoring a minimal configset under test-external-client (e.g., src/test/resources/configsets/...) and resolving it relative to the project/test resources instead of the repo checkout. ########## test-external-client/README.md: ########## @@ -0,0 +1,44 @@ +# Solr dependency smoke test (Maven + Gradle) + +This mini project validates that a standalone build can resolve Solr artifacts from a Maven-style local directory tree and run a basic test using them. + +## Inputs + +- `solr.version` (required): version of `solr-solrj` and `solr-test-framework` +- `local.solr.repo` (optional): filesystem path to a Maven-layout repository (default: `~/.m2/repository`) + Review Comment: The README states the default for local.solr.repo is ~/.m2/repository, but the Gradle build defaults local.solr.repo to $user.dir/build/maven-local. Please reconcile/clarify the defaults (and note Maven vs Gradle differences if intentional) to avoid confusing users running the smoke test. ########## dev-tools/scripts/smokeTestRelease.py: ########## @@ -788,6 +788,66 @@ def testSolrExample(binaryDistPath, javaPath, isSlim): os.chdir(old_cwd) +def findMaven(): + """Find the mvn executable in PATH. Returns the command path, or None if not found.""" + import shutil as shutil_util + return shutil_util.which('mvn') + + +def _dockerAvailable(): + """Check whether Docker is installed and the daemon is running.""" + import shutil as shutil_util + if shutil_util.which('docker') is None: + return False + return os.system('docker info > /dev/null 2>&1') == 0 + + +def testMavenBuild(mavenDir, tmpDir, version): + """ + Runs the test-external-client project with both Maven and Gradle to verify that the + published POMs for solr-solrj and solr-test-framework declare correct transitive + dependencies. + + mavenDir: root of the local Maven repository (contains org/apache/solr/...) + tmpDir: temp directory for log files + version: Solr version string (e.g. "10.0.0") + """ + print(' test external client project (verify POMs are consumable)...') + + scriptDir = os.path.dirname(os.path.abspath(__file__)) + projectDir = os.path.normpath(os.path.join(scriptDir, '..', '..', 'test-external-client')) + if not os.path.isdir(projectDir): + raise RuntimeError('test-external-client directory not found at: %s' % projectDir) + + # Run Maven build + mvnCmd = findMaven() + if mvnCmd is not None: + print(' using local Maven: %s' % mvnCmd) + run('%s -B -f "%s/pom.xml" -Dsolr.version="%s" -Dlocal.solr.repo="%s" test' + % (mvnCmd, projectDir, version, mavenDir), os.path.join(tmpDir, 'maven-build.log')) + elif _dockerAvailable(): + print(' Maven not found; using Docker Maven image...') + # Note: the Docker image already includes Java 21 + # Project is mounted writable so Maven can write its build output directory + run('docker run --rm' + ' -v "%s":/project' + ' -v "%s":/solr-local-release:ro' + ' maven:3.9-eclipse-temurin-21' + ' mvn -B -f /project/pom.xml -Dsolr.version="%s" -Dlocal.solr.repo=/solr-local-release test' + % (projectDir, mavenDir, version), os.path.join(tmpDir, 'maven-build.log')) Review Comment: When falling back to Docker for Maven, the container runs as root by default and will write build outputs into the bind-mounted /project directory, potentially leaving root-owned files in the working copy. Consider adding a user mapping (e.g., -u) or directing Maven’s local repo/target dir to a container-only path to avoid permission issues for subsequent local builds. ########## test-external-client/pom.xml: ########## @@ -0,0 +1,85 @@ +<?xml version="1.0" encoding="UTF-8"?> +<!-- + 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 + + 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. +--> +<project xmlns="http://maven.apache.org/POM/4.0.0"; + xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"; + xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 https://maven.apache.org/xsd/maven-4.0.0.xsd";> + <modelVersion>4.0.0</modelVersion> + + <groupId>org.apache.solr</groupId> + <artifactId>test-external-client</artifactId> + <version>1.0-SNAPSHOT</version> + <name>Solr Dependency Smoke Test</name> + + <properties> + <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding> + <maven.compiler.release>21</maven.compiler.release> + <local.solr.repo>${user.home}/.m2/repository</local.solr.repo> + <solr.version>11.0.0-SNAPSHOT</solr.version> + </properties> Review Comment: The pom relies on Maven’s default maven-compiler-plugin version while setting maven.compiler.release=21. On systems with an older default compiler plugin, this can fail even if the rest of the test is fine (and Docker fallback won’t trigger if mvn exists). Pin a modern maven-compiler-plugin version (and/or enforce a minimum Maven version) to make this smoke test more deterministic. ########## dev-tools/scripts/smokeTestRelease.py: ########## @@ -788,6 +788,66 @@ def testSolrExample(binaryDistPath, javaPath, isSlim): os.chdir(old_cwd) +def findMaven(): + """Find the mvn executable in PATH. Returns the command path, or None if not found.""" + import shutil as shutil_util + return shutil_util.which('mvn') + + +def _dockerAvailable(): + """Check whether Docker is installed and the daemon is running.""" + import shutil as shutil_util + if shutil_util.which('docker') is None: + return False + return os.system('docker info > /dev/null 2>&1') == 0 + + +def testMavenBuild(mavenDir, tmpDir, version): + """ + Runs the test-external-client project with both Maven and Gradle to verify that the + published POMs for solr-solrj and solr-test-framework declare correct transitive + dependencies. + + mavenDir: root of the local Maven repository (contains org/apache/solr/...) + tmpDir: temp directory for log files + version: Solr version string (e.g. "10.0.0") + """ + print(' test external client project (verify POMs are consumable)...') + + scriptDir = os.path.dirname(os.path.abspath(__file__)) + projectDir = os.path.normpath(os.path.join(scriptDir, '..', '..', 'test-external-client')) + if not os.path.isdir(projectDir): + raise RuntimeError('test-external-client directory not found at: %s' % projectDir) + + # Run Maven build + mvnCmd = findMaven() + if mvnCmd is not None: + print(' using local Maven: %s' % mvnCmd) + run('%s -B -f "%s/pom.xml" -Dsolr.version="%s" -Dlocal.solr.repo="%s" test' + % (mvnCmd, projectDir, version, mavenDir), os.path.join(tmpDir, 'maven-build.log')) + elif _dockerAvailable(): + print(' Maven not found; using Docker Maven image...') + # Note: the Docker image already includes Java 21 + # Project is mounted writable so Maven can write its build output directory + run('docker run --rm' + ' -v "%s":/project' + ' -v "%s":/solr-local-release:ro' + ' maven:3.9-eclipse-temurin-21' + ' mvn -B -f /project/pom.xml -Dsolr.version="%s" -Dlocal.solr.repo=/solr-local-release test' + % (projectDir, mavenDir, version), os.path.join(tmpDir, 'maven-build.log')) + else: + print(' WARNING: Neither Maven nor Docker found; skipping Maven build.') + print(' Install Maven or Docker to enable this check.') + + # Also run Gradle build + gradlew = os.path.normpath(os.path.join(projectDir, '..', 'gradlew')) + print(' using Gradle: %s' % gradlew) + run('"%s" --no-daemon -p "%s" -Psolr.version="%s" -Plocal.solr.repo="%s" test' + % (gradlew, projectDir, version, mavenDir), os.path.join(tmpDir, 'gradle-build.log')) + + print(' external client project: SUCCESS') Review Comment: testMavenBuild() prints "external client project: SUCCESS" even when the Maven build is skipped due to missing Maven/Docker. That output can be misleading when reading smoke test logs; consider either failing the check, or explicitly reporting that only the Gradle-based consumption test ran. ########## test-external-client/build.gradle.kts: ########## @@ -0,0 +1,66 @@ +/* + * 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 + * + * 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. + */ + +plugins { + java +} + +group = "org.apache.solr" +version = "1.0-SNAPSHOT" +description = "A smoke test for solr-test-framework & SolrJ dependencies and usage." + +val solrVersion = providers.gradleProperty("solr.version").orElse("11.0.0-SNAPSHOT") +val localSolrRepo = + providers.gradleProperty("local.solr.repo") + .orElse("${System.getProperty("user.dir")}/build/maven-local") Review Comment: local.solr.repo defaults to ${System.getProperty("user.dir")}/build/maven-local, which conflicts with the README’s stated default (~/.m2/repository) and differs from the Maven pom default. Either align defaults across build tools or document why they differ. ########## dev-tools/scripts/smokeTestRelease.py: ########## @@ -788,6 +788,66 @@ def testSolrExample(binaryDistPath, javaPath, isSlim): os.chdir(old_cwd) +def findMaven(): + """Find the mvn executable in PATH. Returns the command path, or None if not found.""" + import shutil as shutil_util + return shutil_util.which('mvn') + + +def _dockerAvailable(): + """Check whether Docker is installed and the daemon is running.""" + import shutil as shutil_util + if shutil_util.which('docker') is None: + return False + return os.system('docker info > /dev/null 2>&1') == 0 + + +def testMavenBuild(mavenDir, tmpDir, version): + """ + Runs the test-external-client project with both Maven and Gradle to verify that the + published POMs for solr-solrj and solr-test-framework declare correct transitive + dependencies. + + mavenDir: root of the local Maven repository (contains org/apache/solr/...) + tmpDir: temp directory for log files + version: Solr version string (e.g. "10.0.0") + """ + print(' test external client project (verify POMs are consumable)...') + + scriptDir = os.path.dirname(os.path.abspath(__file__)) + projectDir = os.path.normpath(os.path.join(scriptDir, '..', '..', 'test-external-client')) + if not os.path.isdir(projectDir): + raise RuntimeError('test-external-client directory not found at: %s' % projectDir) + + # Run Maven build + mvnCmd = findMaven() + if mvnCmd is not None: + print(' using local Maven: %s' % mvnCmd) + run('%s -B -f "%s/pom.xml" -Dsolr.version="%s" -Dlocal.solr.repo="%s" test' Review Comment: The Maven invocation interpolates mvnCmd without quoting. If PATH resolves mvn to a location containing spaces, the command will fail. Consider quoting mvnCmd (or switching run() to use subprocess with an argv list) for more robust execution. -- 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]
