Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[🚀 Feature]: Selenium Manager check drivers on PATH #11356

Closed
titusfortner opened this issue Dec 3, 2022 · 15 comments
Closed

[🚀 Feature]: Selenium Manager check drivers on PATH #11356

titusfortner opened this issue Dec 3, 2022 · 15 comments
Labels
I-enhancement Something could be better
Milestone

Comments

@titusfortner
Copy link
Member

titusfortner commented Dec 3, 2022

Dependencies

Feature and motivation

Right now each of the bindings are processing drivers in this order:

  1. User provided specific location
  2. If driver isn't specified, it is looked for in each directory listed in PATH environment variable
  3. If not provided and not found in PATH, tries to get Selenium Manager to find it

We've told users to try Selenium Manager by deleting drivers and stop using 3rd party managers. One issue we've seen is that there are users with old versions of drivers in directories on their PATH that they do not know about (#11269).

We can fix this, as well as dramatically increase the usage of Selenium Manager by having Selenium Manager check if a driver is on PATH, and checking its version; if the version is compatible, use that one; if it isn't, use Selenium Manager to download the right one (and raise a warning that an old one exists).

Bindings: because not everyone can use Selenium Manager, yet, these will still also need to check PATH, but will do so *after Selenium Manager errors, not before like now.

Note: I would love for Selenium bindings not to have to ever deal with PATH again, but we're a long way out from being able to use Selenium Manager for everything by itself.

Considerations

  1. Outdated driver is found on PATH
    • Raise Error — this was my initial suggestion for this feature in beta, and now I don't like it
    • Log Warning and use it — Based on "don't remediate" I think this is the right answer for now
    • Ignore it and download new one — this is what we are moving to
  2. Selenium Manager Error
    • Bindings catch error and try locating on PATH — This is safe, but probably overkill
    • Bindings no longer do PATH and users are directed to other fixes — This is easier
@titusfortner titusfortner added C-py Python Bindings C-rb Ruby Bindings C-dotnet .NET Bindings C-java Java Bindings C-nodejs JavaScript Bindings I-enhancement Something could be better C-rust Rust code is mostly Selenium Manager labels Dec 3, 2022
@titusfortner
Copy link
Member Author

Important to note that this will depend on #11351 so we know for sure which browser we need to be matching driver versions for.

@titusfortner
Copy link
Member Author

titusfortner commented Apr 21, 2023

Ok, so all of the prereqs I discussed have been implemented...

Current Behavior:

  1. If user has specified location of driver, or driver exists on PATH - use it & ignore SM
  2. Otherwise use Selenium Manager
  3. Catch any exceptions
    • Throw Exception with the "old" error message to put on PATH

Desired Behavior:

  1. If user has specified the location of the driver, use it & ignore SM
  2. Selenium Manager looks for driver on PATH
    • If driver found on PATH & it is compatible - use it
    • If driver found on PATH & not compatible - ignore it
  3. Otherwise use Selenium Manager to download/cache correct driver
  4. Catch any exceptions
    • Throw custom exception based on SM failure details & link to documentation

Proposed Beta Behavior:

  1. If user has specified the location of the driver, use it & ignore SM
  2. Selenium Manager looks for driver on PATH
    • If driver found on PATH & it is compatible - use it
    • If driver found on PATH & not compatible - log warning & raise exception use it
  3. Otherwise use Selenium Manager to download/cache correct driver
  4. Catch any exceptions
    • Look for driver on PATH & use it (might fail)
    • Throw Exception with message linking to Driver Installation Documentation

@titusfortner
Copy link
Member Author

titusfortner commented May 19, 2023

I think this depends on #11639

@titusfortner
Copy link
Member Author

Also, I just found another blocker, this one in Rust.

If a valid driver is available on PATH, but the computer does not have internet access. Selenium Manager binary errors out instead of using it:

2023-05-24 07:12:13 ERROR Selenium [:selenium_manager] error sending request for url (https://chromedriver.storage.googleapis.com/LATEST_RELEASE_113): error trying to connect: dns error: failed to lookup address information: nodename nor servname provided, or not known

@diemol diemol modified the milestones: 4.10, 4.11 May 24, 2023
@titusfortner
Copy link
Member Author

Everything on the bindings side should be super easy now that we've decided to take the easy path here. The only one that is going to be tough is .NET because the DriverFinder class is evaluating potentially previously found driver on PATH rather than looking for it directly, so more logic will need to be changed.

@diemol
Copy link
Member

diemol commented Jul 13, 2023

Why is #11639 a dependency?

@diemol diemol removed the C-py Python Bindings label Jul 13, 2023
@titusfortner
Copy link
Member Author

For grid. We need to enable SM so it will find drivers on path, but not download new drivers.

@diemol diemol removed the C-dotnet .NET Bindings label Jul 13, 2023
@titusfortner titusfortner removed the C-rb Ruby Bindings label Jul 13, 2023
@titusfortner titusfortner removed their assignment Jul 13, 2023
@titusfortner titusfortner removed the C-rust Rust code is mostly Selenium Manager label Jul 19, 2023
@diemol diemol added B-grid Everything grid and server related and removed C-java Java Bindings labels Jul 21, 2023
diemol added a commit that referenced this issue Jul 21, 2023
@diemol diemol removed the B-grid Everything grid and server related label Jul 21, 2023
diemol added a commit that referenced this issue Jul 21, 2023
@diemol diemol removed the C-nodejs JavaScript Bindings label Jul 21, 2023
@diemol
Copy link
Member

diemol commented Jul 21, 2023

Implemented in all bindings now.

@cml37
Copy link

cml37 commented Aug 5, 2023

This change is going to break all architectures that do not have a selenium-manager compatible binary (for example Raspbian with armv7l) without intervention. It may be worthwhile to roll it back.

Since I use a Java implementation of selenium on Raspbian, I can do the following to work around this for the time being:

        ChromeDriverService service = (new ChromeDriverService.Builder())
                .usingDriverExecutable(new File(new ExecutableFinder()
                        .find(ChromeDriverService.CHROME_DRIVER_NAME)))
                .build();
        ChromeDriver driver = new ChromeDriver(service);

@titusfortner
Copy link
Member Author

There are so few people on armv7l that we believe the tradeoff is worth it overall. Please set the location to the driver with the system property or with a method on a service instance. Sorry for the extra work.

@cml37
Copy link

cml37 commented Aug 5, 2023

Yea, you are probably right, this probably impacted only a few people, and I can just use the ExecutableFinder. Alternatively if you can detect that the architecture is unsupported, you could catch this upstream and use the ExecutableFinder there, it's worthy of consideration!

@titusfortner
Copy link
Member Author

Well, I'm trying to get it so we can remove all the executable finder code in all the languages 😂

Manual management and specifying the location is likely the solution for foreseeable future.

Perhaps if we add a feature like #11359 you might be able to build your own selenium manager on your architecture, would need rust to compile it. But then we'd have to point it to electron to get the driver?

@cml37
Copy link

cml37 commented Aug 5, 2023

Totally makes sense!

I might hack a bit, I did get selenium-manager to build just now!

pi@pitest:~/selenium/rust $ ./target/armv7-unknown-linux-gnueabihf/release/selenium-manager -V
selenium-manager 1.0.0-M4

Steps:

curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh
source $HOME/.cargo/env
sudo apt-get install snapd
sudo snap install zig --beta --classic
export RUSTFLAGS='-Ctarget-feature=-crt-static'
cargo install cargo-zigbuild
rustup target add armv7-unknown-linux-gnueabihf
git clone --single-branch https://github.com/SeleniumHQ/selenium
cd selenium/rust
cargo zigbuild --release --target armv7-unknown-linux-gnueabihf

@cml37
Copy link

cml37 commented Aug 6, 2023

Okay, going to punt a bit for now, since selenium-manager isn't really set up to deploy or to look for Chromium it seems (it tried to download an x64 version of Chrome.. also, I don't believe that there is a version of Chrome for Raspberry Pi). So, suppose I will use the ExecutableManager for now, and then eventually will have to just hack in a path to the chromedriver in the future.

However, if there is interest in expanding selenium-manager to support these use cases, I'll be standing by to test!

Copy link

github-actions bot commented Dec 9, 2023

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 github-actions bot locked and limited conversation to collaborators Dec 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
I-enhancement Something could be better
Projects
Status: Done
Development

No branches or pull requests

3 participants