selenium icon indicating copy to clipboard operation
selenium copied to clipboard

[🐛 Bug]: org.openqa.selenium.os.CommandLine.java class does not check the executable in same ENV that it will execute in.

Open alexander-poulikakos opened this issue 2 years ago • 1 comments

What happened?

I'm running the following Java code:

		CommandLine cl = new CommandLine("node", "-v");
		cl.setEnvironmentVariable("PATH", "/opt/homebrew/bin");
		cl.execute();
		System.out.println(cl.getStdOut());

Which, on my machine (a MAC), throws java.lang.NullPointerException: Unable to find executable for: node exception.

I'm expecting above code to print the current node version in the console. Example:

v18.9.0

On my machine the env PATH looks like this:

System.out.println(System.getenv("PATH"));

output:

/usr/bin:/bin:/usr/sbin:/sbin

node is located under this folder: /opt/homebrew/bin (This is where brew installs it as of lately.

Error reason: When instantiating the CommandLine class, an instance of OsProcess is created here. In the constructor of OsProcess a check is made here to verify the executable can be found. This is done by the ExecutableFinder class here.

The problem is that the ExecutableFinder is only considering looking in the paths provided by System.getenv("PATH"). When the OsProcess will execute the executable here, it will use the "PATH" provided by the user (as provided in above java example). There is a mismatch which env "PATH" is used by ExecutableFinder and OsProcess. The same env "PATH" should be used for checking the executable AND running the executable.

How can we reproduce the issue?

.

Relevant log output

.

Operating System

macOS version 12.3

Selenium version

selenium-remote-driver-3.141.59.jar

What are the browser(s) and version(s) where you see this issue?

N/A

What are the browser driver(s) and version(s) where you see this issue?

selenium-remote-driver-3.141.59.jar

Are you using Selenium Grid?

No response

alexander-poulikakos avatar Sep 20 '22 11:09 alexander-poulikakos

@alexander-poulikakos, thank you for creating this issue. We will troubleshoot it as soon as we can.


Info for maintainers

Triage this issue by using labels.

If information is missing, add a helpful comment and then I-issue-template label.

If the issue is a question, add the I-question label.

If the issue is valid but there is no time to troubleshoot it, consider adding the help wanted label.

If the issue requires changes or fixes from an external project (e.g., ChromeDriver, GeckoDriver, MSEdgeDriver, W3C), add the applicable G-* label, and it will provide the correct link and auto-close the issue.

After troubleshooting the issue, please add the R-awaiting answer label.

Thank you!

github-actions[bot] avatar Sep 20 '22 11:09 github-actions[bot]

Thank you for the details. The selenium version you are using is no longer maintained. Can you please try the latest version https://www.selenium.dev/downloads/ and provide an update if the problem still persists? Thank you!

pujagani avatar Sep 22 '22 05:09 pujagani

Yes, same thing happens with selenium-remote-driver-4.4.0.jar

alexander-poulikakos avatar Sep 22 '22 07:09 alexander-poulikakos

While I am triaging and figuring out what is the right way to tackle this, meanwhile please use the example below to make sure your code works.

 CommandLine cl = new CommandLine(""/opt/homebrew/bin/node", "-v");
 cl.execute();
 System.out.println(cl.getStdOut());

pujagani avatar Sep 22 '22 13:09 pujagani

Yes, running this works:

		CommandLine cl = new CommandLine("/opt/homebrew/bin/node", "-v");
		cl.execute();
		System.out.println(cl.getStdOut());

But in my use case, I'm not using the selenium code directly. I'm using the Appium Java-client. More specifically the AppiumServiceBuilder class. so can't change the "node" string.

It seems the problem is within the ExecutableFinder as it hardcodes the env PATH to this

Map<String, String> env = System.getenv();

Maybe there needs to be a way to set a custom env PATH to ExecutableFinder.

alexander-poulikakos avatar Sep 22 '22 14:09 alexander-poulikakos

Thank you for the update and the details. From what I understand the way the CommandLine and OsProcess class is implemented, the intent is not to overwrite the PATH variable. When the constructor is initialized, it first finds the executable, and then there is an option to set the environment variables which is used to run the executable but not find it. Even the tests for the above classes are written the same way.

I understand your use case but for homebrew, ideally, it should be added to the system path and not via code. So homebrew-specific installed modules can be accessed directly via the command line.

pujagani avatar Sep 23 '22 04:09 pujagani

I will try to see if there is a way a custom PATH can be provided and appended to the existing one without changing the way things are designed but I am not sure if PATH should be modified by code.

pujagani avatar Sep 23 '22 04:09 pujagani

I just discovered that AppiumServiceBuilder class provides an environment variable that is used to locate the Node executable. https://github.com/appium/java-client/blob/fe17e854ad61605e2f4e1988fde2cd5615cfaf2b/src/main/java/io/appium/java_client/service/local/AppiumServiceBuilder.java#L75 Based on my understanding, you can just use that for your requirement and that should do the trick. I do not think this is a bug in Selenium and Appium provides a way to achieve what you want.

pujagani avatar Sep 23 '22 05:09 pujagani

Yes, there is a NODE_BINARY_PATH system/env variable available in AppiumServiceBuilder that I can set, but it requires the full path. Which means I need to duplicate the find executable mechanism on my side. Would be nicer if Appium could reuse the already existing ExecutableFinder in selenium.

Maybe not a bug in ExecutableFinder, but could it be a feature request instead?

alexander-poulikakos avatar Sep 23 '22 07:09 alexander-poulikakos

I am not aware of how you are using the AppiumServiceBuilder, but I am not sure I fully understand the concern around providing the full path and the need to duplicate the find executable mechanism. If we need to add a method in ExecutableFinder that Appium can later use or integrate that can be done. But I do not think CommandLine or OsProcess class should be changed apart from adding a constructor to it that can pass on the path to look into and eventually go to the new method in the ExecutableFinder. Does that solution address your use-case?

pujagani avatar Sep 23 '22 07:09 pujagani

When reporting this, I was using an old version of Appium java client. Which was doing this. I.e using the selenium CommandLine class (which internally uses ExecutableFinder).

However, in latest version of AppiumServiceBuilder that has changed to this. I.e using the ExecutableFinder directly (so no changes needed in CommandLine or OsProcess).

Would be nice if the env could be passed into Constructor of ExecutableFinder. This will later need to be integrated into Appium java client.

I have tried java 8, 9, and 11 and I am unable to see the error. I will mainly trying Java 9 since module related issues do show up there.

This code:

	public static void main(String[] args) {
		String fullPath = new ExecutableFinder().find("node");
		System.out.println("full-path: " + fullPath);
		System.out.println("exists: " + new File("/opt/homebrew/bin/node").exists());
		System.out.println("env PATH: " + System.getenv("PATH"));
		System.out.println("java-version: " + System.getProperty("java.version"));
	}

prints this on my machine (OSX):

full-path: null
exists: true
env PATH: /usr/bin:/bin:/usr/sbin:/sbin
java-version: 11.0.16

I would expect running something like this (note env passed into constructor):

	public static void main(String[] args) {
		Map<String, String> env = ... // where PATH = "/opt/homebrew/bin:..."
		String fullPath = new ExecutableFinder(env).find("node");
		System.out.println("full-path: " + fullPath);
		System.out.println("exists: " + new File("/opt/homebrew/bin/node").exists());
		System.out.println("env PATH: " + System.getenv("PATH"));
		System.out.println("java-version: " + System.getProperty("java.version"));
	}

would output this:

full-path: /opt/homebrew/bin/node
exists: true
env PATH: /usr/bin:/bin:/usr/sbin:/sbin
java-version: 11.0.16

PS. I would not mind providing a PR for the change, but I have not been able to figure out how to import selenium java project into any IDE (Intellij or Eclipse) and be able to run the unit tests. Any hints how to do that? Any tutorials? Most modern java projects usually use either maven or gradle, but I don't think that is the case for Selenium.

alexander-poulikakos avatar Sep 23 '22 09:09 alexander-poulikakos

Sincere apologies for I have tried java 8, 9, and 11 and I am unable to see the error. I will mainly trying Java 9 since module related issues do show up there. This was for another issue. I did not realize I wrote it here. I have deleted it to avoid confusion. I appreciate your detailed response. This sounds good. We can add such a constructor and as you mentioned, Appium can help integrate it. Contributions are always welcome! Selenium uses bazel. For IntelliJ, I use bazel plugin which allows importing bazel a project and that allows running tests. You can also use commandline bazel commands https://github.com/SeleniumHQ/selenium#java to run tests. I can always help out as needed.

pujagani avatar Sep 23 '22 12:09 pujagani

I will close this because we never got the PR, but I also believe these classes are meant for internal usage with the exception of the Appium project. However, we are always happy to revisit this when a PR comes that is linked to this issue.

diemol avatar Jun 12 '23 13:06 diemol

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

github-actions[bot] avatar Dec 09 '23 00:12 github-actions[bot]