ropensci / RSelenium

An R client for Selenium Remote WebDriver
https://docs.ropensci.org/RSelenium
343 stars 81 forks source link

Auto-detect proper ChromeDriver version #237

Open salim-b opened 4 years ago

salim-b commented 4 years ago

This PR adds a modified version of the logic proposed in #221.

Two additional packages were imported: xfun for OS detection and stringr for regex handling (base R's functions are a PITA for string extraction).

Besides, some other small changes were made: All the commits before db3bb46 are de-facto unrelated clean-ups/minor improvements.

I'm not really familiar with the RSelenium/wdman/binman functionality separation. Maybe wdman might actually be the better place to add the ChromeDriver auto-detection. In that case the code I added could be easily moved.

aaronrudkin commented 3 years ago

Not really sure who is involved with maintaining the package at ropensci, but this is a very useful PR.

We're using RSelenium as part of a college text-as-data course and by far the most common bugs students run into are:

  1. Java runtime not installed
  2. Chromedriver selects driver version > browser version (solved by this PR)
  3. Mac Selenium downloading M1 drivers on Intel macs (discussed in #240 and solved by JeffreyBLewis's PR to wdman)
  4. Improper freeing of ports after server destruction on Windows (partial mitigation by auto-selecting a free port in #224, other strategies described in comments section of #228)

Recognizing that Selenium itself is a little clunky and that package maintainers have suggested people move to running Selenium inside Docker rather than through the rsDriver() method, it'd still be handy to get these PRs merged and the functionality for the fourth issue above implement.

How can @salim-b or I assist with getting this PR merged?

Danny-dK commented 3 years ago

Does this work for as long as the PR is not merged (at least for Windows, but I assume it can be adjusted for other systems)? https://github.com/ropensci/RSelenium/issues/198#issuecomment-527799287

salim-b commented 3 years ago

Does this work for as long as the PR is not merged (at least for Windows, but I assume it can be adjusted for other systems)? #198 (comment)

Sorry, I don't quite understand your question. This PR should work for Linux, macOS and Windows. What exactly would you like to know?

Danny-dK commented 3 years ago

Ah I misunderstood. I thought the code in the comment I posted would work for people having trouble with Chrome drivers for as long as this PR still isn't merged (who will be merging this?). I can now see in the work you have done that people could implement that as well (even better) even as long as this PR is not merged yet.

salim-b commented 3 years ago

who will be merging this?

I don't know, the maintainer(s) of this repository could. But honestly, it appears this R package is pretty much abandoned. Obviously, I don't have write access to the repository.

as long as this PR still isn't merged

You can install a version of RSelenium that includes this PR using:

remotes::install_github("ropensci/RSelenium#237")
MarkMc1089 commented 3 years ago

I tried out remotes::install_github("ropensci/RSelenium#237"). My Chrome is up to date, according to the About info in Settings, version 95. However the error message below indicates the version of RSeleniumworks only with Chrome version 96. Additionally the warning message Installed stable Google Chrome browser version couldn't be determined. is incorrect, as the version of Chrome I have was determined.

Selenium message:session not created: This version of ChromeDriver only supports Chrome version 96
Current browser version is 95.0.4638.69 with binary path C:\Program Files\Google\Chrome\Application\chrome.exe
Build info: version: '4.0.0-alpha-2', revision: 'f148142cf8', time: '2019-07-01T21:30:10'
System info: host: 'BSA-L-7V9VZ33', ip: '192.168.0.19', os.name: 'Windows 10', os.arch: 'amd64', os.version: '10.0', java.version: '1.8.0_211'
Driver info: driver.version: unknown
remote stacktrace: Backtrace:
    Ordinal0 [0x00546023+2514979]
    Ordinal0 [0x004DF6B1+2094769]
    Ordinal0 [0x003E26C8+1058504]
    Ordinal0 [0x00400A0D+1182221]
    Ordinal0 [0x003FC8D0+1165520]
    Ordinal0 [0x003FA13F+1155391]
    Ordinal0 [0x0042A8EF+1353967]
    Ordinal0 [0x0042A55A+1353050]
    Ordinal0 [0x004261EB+1335787]
    Ordinal0 [0x00402617+1189399]
    Ordinal0 [0x00403479+1193081]
    GetHandleVerifier [0x006D5624+1579748]
    GetHandleVerifier [0x00780417+2279639]
    GetHandleVerifier [0x005D473B+527355]
    GetHandleVerifier [0x005D37E9+523433]
    Ordinal0 [0x004E4BF9+2116601]
    Ordinal0 [0x004E9238+2134584]
    Ordinal0 [0x004E9372+2134898]
    Ordinal0 [0x004F2EB1+2174641]
    BaseThreadInitThunk [0x7663FA29+25]
    RtlGetAppContainerNamedObjectPath [0x77857A9E+286]
    RtlGetAppContainerNamedObjectPath [0x77857A6E+238]

Could not open chrome browser.
Client error message:
     Summary: SessionNotCreatedException
     Detail: A new session could not be created.
     Further Details: run errorDetails method
Check server log for further details.
Warning message:
In rsDriver(port = 55657L, browser = "chrome") :
  Installed stable Google Chrome browser version couldn't be determined. Falling back to `chromever = "latest"`.
salim-b commented 3 years ago

However the error message below indicates the version of RSeleniumworks only with Chrome version 96.

I guess that is simply the truth. It means the code I've added to auto-choose the right ChromeDriver version somewhere failed and it was tried to use a 96.x ChromeDriver version with your 95.0.4638.69 version of Chrome browser.

Additionally the warning message Installed stable Google Chrome browser version couldn't be determined. is incorrect, as the version of Chrome I have was determined.

Nah, you're misreading the output here. The warning message comes from the code I've added and means the function systemChromeVersion() I've added couldn't determine your system's Chrome version. The other output comes from Selenium itself after it fails.

You can verify that by running:

RSelenium:::systemChromeVersion()

If you get an empty character vector (character(0)), it means systemChromeVersion() didn't do its job as supposed.

On Windows, its tried to detect the Chrome browser version using

system2( 
  command = "wmic", 
  args = 'datafile where name="C:\\\\Program Files (x86)\\\\Google\\\\Chrome\\\\Application\\\\chrome.exe" get Version /value', 
  stdout = TRUE, 
  stderr = TRUE 
) 

According to your output, it seems there's an x64 version of Chrome now, so the above code is simply looking in the wrong directory for chrome.exe.

Thus, could you please run the following and tell me if it successfully returns a version string?

system2( 
  command = "wmic", 
  args = 'datafile where name="C:\\\\Program Files\\\\Google\\\\Chrome\\\\Application\\\\chrome.exe" get Version /value', 
  stdout = TRUE, 
  stderr = TRUE 
) 
MarkMc1089 commented 3 years ago

You were right @salim-b:

RSelenium:::systemChromeVersion()

Error in get(name, envir = asNamespace(pkg), inherits = FALSE) : 
  object 'systemChromeVersion' not found

and looking for the x64 version did find it:

system2( 
    command = "wmic", 
    args = 'datafile where name="C:\\\\Program Files\\\\Google\\\\Chrome\\\\Application\\\\chrome.exe" get Version /value', 
    stdout = TRUE, 
    stderr = TRUE 
)

[1] "\r"                     "\r"                     "Version=95.0.4638.69\r" "\r"                     "\r"                     "\r"

Not sure if you expected all those "\r"' ?

salim-b commented 3 years ago

@MarkMc1089 Thanks for the confirmation about the x64 version! I've added support for the x64 Chrome binary on Windows.

But NOTE that the message

Error in get(name, envir = asNamespace(pkg), inherits = FALSE) : 
  object 'systemChromeVersion' not found

suggests you're not actually using a version of RSelenium that includes this PR since the function (object) systemChromeVersion was introduced by this very PR. 😬

Try re-installing the latest version of this PR by

if (!("remotes" %in% rownames(installed.packages()))) {
  install.packages(pkgs = "remotes",
                   repos = "https://cloud.r-project.org/")
}

remotes::install_github("ropensci/RSelenium#237")

and report back if it works now :slightly_smiling_face:

MarkMc1089 commented 3 years ago

Thanks for making some updates! It is now working using your fork, tho there is a warning (coming from systemChromeVersion():

rD <- RSelenium::rsDriver(port = netstat::free_port(), browser = "chrome", verbose = FALSE)
Warning message:
In if (is.na(chrome_version)) { :
  the condition has length > 1 and only the first element will be used

Helpful note: I recommend using the netstatpackage's free_port - this saved me from having to change the port every time I try running RSelenium!

Hopefully your PR wil be accepted soon. How can I ensure I get an update if it does, so I can reinstall from this repo?

Danny-dK commented 3 years ago

@MarkMc1089 Would the free_port() function eventually lead to a memory issue if you rerun Rselenium many times without closing the port? If you add a section in the script to close the port, you don't need to change the port number. Previously this would close it:

rD1 <- rsDriver(browser = "chrome", port=4567L, chromever='latest')

remDr1 <- rD1[["client"]] 

remDr1$close()
rD1$server$stop() 
rm(rD1)
gc()

But as of 2020 there seems to be an issue where this does not close it properly. One way about it is to use the following suggestion from https://github.com/ropensci/RSelenium/issues/228#issuecomment-632885370 :

system("taskkill /im java.exe /f", intern=FALSE, ignore.stdout=FALSE)
salim-b commented 3 years ago

Thanks for making some updates! It is now working using your fork, tho there is a warning (coming from systemChromeVersion():

Thanks for reporting back, and glad it basically works now. The warning should be fixed now (just re-install using remotes::install_github("ropensci/RSelenium#237")).

Hopefully your PR wil be accepted soon.

I have no clue if this is ever gonna happen. No activity from package maintainers since 1 Feb 2020 on this repository...

How can I ensure I get an update if it does, so I can reinstall from this repo?

You could watch (subscribe to) this repo for changes, so you receive notifications when something happens you don't want to miss.

salim-b commented 3 years ago

@Danny-dK @MarkMc1089 I'd be glad if you could move discussion about this to a dedicated place (like #228) since it's unrelated to this PR. Thanks.

Danny-dK commented 3 years ago

@salim-b Sorry about that, will do. I contacted one of the contributors and he said that the maintainer does not seem to be active anymore. He indicated that the package is 'up for adoption'. If you're interested, you can contact https://github.com/jeroen.

salim-b commented 3 years ago

He indicated that the package is 'up for adoption'. If you're interested, you can contact https://github.com/jeroen.

To be honest, I don't feel familiar enough with the package internals to take over complete maintainership.