Copilot commented on code in PR #72:
URL: https://github.com/apache/maven-jdeps-plugin/pull/72#discussion_r2647918222
##########
src/main/java/org/apache/maven/plugins/jdeps/AbstractJDepsMojo.java:
##########
@@ -392,22 +392,37 @@ private String getJDepsExecutable() throws IOException {
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
+ //
----------------------------------------------------------------------
+ Properties env = CommandLineUtils.getSystemEnvVars();
+ String path = env.getProperty("PATH");
+ if (path != null) {
+ String[] pathDirs = path.split(File.pathSeparator);
+ for (String pathDir : pathDirs) {
+ File pathJdepsExe = new File(pathDir, jdepsCommand);
+ if (pathJdepsExe.exists() && pathJdepsExe.isFile()) {
Review Comment:
The PATH lookup doesn't verify that the found executable is actually
executable or perform security validation. Consider checking if the file has
executable permissions using `canExecute()` before returning it, to avoid
potential security issues where a non-executable file with the same name could
be found in PATH.
```suggestion
if (pathJdepsExe.exists() && pathJdepsExe.isFile() &&
pathJdepsExe.canExecute()) {
```
##########
src/main/java/org/apache/maven/plugins/jdeps/AbstractJDepsMojo.java:
##########
@@ -392,22 +392,37 @@ private String getJDepsExecutable() throws IOException {
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
+ //
----------------------------------------------------------------------
+ Properties env = CommandLineUtils.getSystemEnvVars();
+ String path = env.getProperty("PATH");
+ if (path != null) {
+ String[] pathDirs = path.split(File.pathSeparator);
+ for (String pathDir : pathDirs) {
+ File pathJdepsExe = new File(pathDir, jdepsCommand);
+ if (pathJdepsExe.exists() && pathJdepsExe.isFile()) {
+ return pathJdepsExe.getAbsolutePath();
Review Comment:
Inconsistent use of canonical paths. The JAVA_HOME path uses
`.getCanonicalFile()` (lines 396, 406) but the PATH lookup does not. For
consistency and to handle symbolic links correctly, consider using
`pathJdepsExe.getCanonicalFile()` in the exists and isFile checks.
```suggestion
File canonicalPathJdepsExe =
pathJdepsExe.getCanonicalFile();
if (canonicalPathJdepsExe.exists() &&
canonicalPathJdepsExe.isFile()) {
return canonicalPathJdepsExe.getAbsolutePath();
```
##########
src/it/path-fallback/invoker.properties:
##########
@@ -0,0 +1,19 @@
+# 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.
+
+invoker.goals = clean compile jdeps:jdkinternals
+invoker.debug = true
Review Comment:
The integration test doesn't appear to actually unset or override the
JAVA_HOME environment variable. The invoker.properties file should include a
property like `invoker.environmentVariables.JAVA_HOME=` or similar to actually
test the PATH fallback scenario when JAVA_HOME is not set. Without this, the
test might be passing due to JAVA_HOME being set rather than testing the PATH
fallback functionality.
##########
src/main/java/org/apache/maven/plugins/jdeps/AbstractJDepsMojo.java:
##########
@@ -392,22 +392,37 @@ private String getJDepsExecutable() throws IOException {
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
+ //
----------------------------------------------------------------------
+ Properties env = CommandLineUtils.getSystemEnvVars();
Review Comment:
The `env` variable is being retrieved twice (once at line 393 and again at
line 411), which is inefficient. Consider reusing the same `env` Properties
object that was already retrieved at line 393 instead of calling
`CommandLineUtils.getSystemEnvVars()` again.
##########
src/main/java/org/apache/maven/plugins/jdeps/AbstractJDepsMojo.java:
##########
@@ -392,22 +392,37 @@ private String getJDepsExecutable() throws IOException {
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
+ //
----------------------------------------------------------------------
+ Properties env = CommandLineUtils.getSystemEnvVars();
+ String path = env.getProperty("PATH");
+ if (path != null) {
+ String[] pathDirs = path.split(File.pathSeparator);
+ for (String pathDir : pathDirs) {
+ File pathJdepsExe = new File(pathDir, jdepsCommand);
+ if (pathJdepsExe.exists() && pathJdepsExe.isFile()) {
+ return pathJdepsExe.getAbsolutePath();
+ }
+ }
+ }
+
+ throw new IOException(
+ "The jdeps executable '" + jdepsExe
+ + "' doesn't exist or is not a file. Verify the
JAVA_HOME environment variable or ensure jdeps is available in PATH.");
Review Comment:
The error message includes the path from `jdepsExe` variable, which may not
be meaningful to users if JAVA_HOME was not set. In that case, `jdepsExe` might
still reference the System Java home path that failed earlier. Consider making
the error message more generic or constructing a more informative message that
doesn't reference a specific invalid path.
```suggestion
"Unable to locate the jdeps executable. Verify that
JAVA_HOME is set correctly or ensure that jdeps is available on the system
PATH.");
```
##########
src/main/java/org/apache/maven/plugins/jdeps/AbstractJDepsMojo.java:
##########
@@ -392,22 +392,37 @@ private String getJDepsExecutable() throws IOException {
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
+ //
----------------------------------------------------------------------
+ Properties env = CommandLineUtils.getSystemEnvVars();
+ String path = env.getProperty("PATH");
+ if (path != null) {
+ String[] pathDirs = path.split(File.pathSeparator);
+ for (String pathDir : pathDirs) {
Review Comment:
The PATH splitting doesn't handle potential edge cases such as empty path
entries that could result from consecutive path separators. Consider filtering
out empty or whitespace-only entries from `pathDirs` before iterating, or
adding a check to skip empty `pathDir` values.
```suggestion
for (String pathDir : pathDirs) {
if (StringUtils.isBlank(pathDir)) {
continue;
}
```
--
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]
