Copilot commented on code in PR #72:
URL: https://github.com/apache/maven-jdeps-plugin/pull/72#discussion_r2647936213
##########
src/main/java/org/apache/maven/plugins/jdeps/AbstractJDepsMojo.java:
##########
@@ -389,25 +389,44 @@ private String getJDepsExecutable() throws IOException {
//
----------------------------------------------------------------------
// Try to find jdepsExe from JAVA_HOME environment variable
//
----------------------------------------------------------------------
+ Properties env = CommandLineUtils.getSystemEnvVars();
if (!jdepsExe.exists() || !jdepsExe.isFile()) {
- Properties env = CommandLineUtils.getSystemEnvVars();
String javaHome = env.getProperty("JAVA_HOME");
- if (StringUtils.isEmpty(javaHome)) {
- throw new IOException("The environment variable JAVA_HOME is
not correctly set.");
- }
- if ((!new File(javaHome).getCanonicalFile().exists())
- || (new File(javaHome).getCanonicalFile().isFile())) {
- throw new IOException("The environment variable JAVA_HOME=" +
javaHome
- + " doesn't exist or is not a valid directory.");
- }
+ if (!StringUtils.isEmpty(javaHome)) {
+ if ((!new File(javaHome).getCanonicalFile().exists())
+ || (new File(javaHome).getCanonicalFile().isFile())) {
+ throw new IOException("The environment variable
JAVA_HOME=" + javaHome
+ + " doesn't exist or is not a valid directory.");
+ }
- jdepsExe = new File(javaHome + File.separator + "bin",
jdepsCommand);
+ jdepsExe = new File(javaHome + File.separator + "bin",
jdepsCommand);
+ }
}
if (!jdepsExe.getCanonicalFile().exists()
|| !jdepsExe.getCanonicalFile().isFile()) {
- throw new IOException("The jdeps executable '" + jdepsExe
- + "' doesn't exist or is not a file. Verify the JAVA_HOME
environment variable.");
+ //
----------------------------------------------------------------------
+ // Try to find jdepsExe from PATH environment variable
+ //
----------------------------------------------------------------------
+ String path = env.getProperty("PATH");
Review Comment:
The PATH environment variable lookup may fail on case-sensitive systems. On
Windows systems, environment variables are case-insensitive, so the PATH
variable might be stored as "Path" or "path". The code should check for common
variations of the PATH variable name to ensure cross-platform compatibility.
```suggestion
String path = env.getProperty("PATH");
if (path == null) {
path = env.getProperty("Path");
}
if (path == null) {
path = env.getProperty("path");
}
```
##########
src/main/java/org/apache/maven/plugins/jdeps/AbstractJDepsMojo.java:
##########
@@ -389,25 +389,44 @@ private String getJDepsExecutable() throws IOException {
//
----------------------------------------------------------------------
// Try to find jdepsExe from JAVA_HOME environment variable
//
----------------------------------------------------------------------
+ Properties env = CommandLineUtils.getSystemEnvVars();
if (!jdepsExe.exists() || !jdepsExe.isFile()) {
- Properties env = CommandLineUtils.getSystemEnvVars();
String javaHome = env.getProperty("JAVA_HOME");
- if (StringUtils.isEmpty(javaHome)) {
- throw new IOException("The environment variable JAVA_HOME is
not correctly set.");
- }
- if ((!new File(javaHome).getCanonicalFile().exists())
- || (new File(javaHome).getCanonicalFile().isFile())) {
- throw new IOException("The environment variable JAVA_HOME=" +
javaHome
- + " doesn't exist or is not a valid directory.");
- }
+ if (!StringUtils.isEmpty(javaHome)) {
+ if ((!new File(javaHome).getCanonicalFile().exists())
+ || (new File(javaHome).getCanonicalFile().isFile())) {
+ throw new IOException("The environment variable
JAVA_HOME=" + javaHome
+ + " doesn't exist or is not a valid directory.");
+ }
- jdepsExe = new File(javaHome + File.separator + "bin",
jdepsCommand);
+ jdepsExe = new File(javaHome + File.separator + "bin",
jdepsCommand);
+ }
}
if (!jdepsExe.getCanonicalFile().exists()
|| !jdepsExe.getCanonicalFile().isFile()) {
- throw new IOException("The jdeps executable '" + jdepsExe
- + "' doesn't exist or is not a file. Verify the JAVA_HOME
environment variable.");
+ //
----------------------------------------------------------------------
+ // Try to find jdepsExe from PATH environment variable
+ //
----------------------------------------------------------------------
+ String path = env.getProperty("PATH");
+ if (path != null) {
+ String[] pathDirs = path.split(File.pathSeparator);
+ for (String pathDir : pathDirs) {
+ if (StringUtils.isBlank(pathDir)) {
+ continue;
+ }
+ File pathJdepsExe = new File(pathDir, jdepsCommand);
+ File canonicalPathJdepsExe =
pathJdepsExe.getCanonicalFile();
+ if (canonicalPathJdepsExe.exists()
+ && canonicalPathJdepsExe.isFile()
+ && canonicalPathJdepsExe.canExecute()) {
+ return canonicalPathJdepsExe.getAbsolutePath();
+ }
+ }
+ }
+
+ throw new IOException(
+ "Unable to locate the jdeps executable. Verify that
JAVA_HOME is set correctly or ensure that jdeps is available on the system
PATH.");
}
Review Comment:
The canExecute() check is only applied when searching the PATH, but not when
jdeps is found in JAVA_HOME or System Java home directory. For consistency and
security, the executable permission check should also be applied to jdeps found
via JAVA_HOME before returning it at line 432. Without this check, the code may
attempt to execute a non-executable file when found via JAVA_HOME.
```suggestion
if (!jdepsExe.canExecute()) {
throw new IOException("The jdeps executable '" + jdepsExe + "'
is not executable.");
}
```
##########
src/it/path-fallback/verify.groovy:
##########
@@ -0,0 +1,32 @@
+/*
+ * 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.
+ */
+
+def buildLog = new File( basedir, 'build.log' )
+
+// Verify that the build succeeded even if JAVA_HOME was not explicitly set
+// This test validates the PATH fallback functionality
+def found = false;
+
+buildLog.eachLine { String line ->
+ if ( line.contains( 'BUILD SUCCESS' ) ) {
+ found = true;
+ }
+}
+
+assert found : "Build should have succeeded using jdeps from PATH"
Review Comment:
The integration test only verifies that the build succeeds but doesn't
confirm that the PATH fallback mechanism was actually used. The test should
verify that jdeps was found in PATH rather than JAVA_HOME by checking the build
log for specific messages or by verifying that JAVA_HOME was indeed unset
during 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]
