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

Fix bug in java versions >9 #103

Open
wants to merge 1 commit into
base: develop
from

Conversation

Projects
None yet
2 participants
@sherrillmix

sherrillmix commented Aug 27, 2018

I think Java is up to double digit versions now. For example, R CMD javareconf on my computer produces:
Java version: 10.0.2

It looks like line 46 of R/utilities.R:

if (jversion < "1.5.0")stop(...)

calls a simple < to check versions which does not parse this correctly. For example "10.0.2" < "1.0.6" evaluates to TRUE in R. It looks like the utils package has a compareVersion function which seems to evaluate correctly e.g. utils::compareVersion("10.0.6","1.5.0") returns 1.

It looks like this package already has dependencies on utils so would this be a simple fix without adding any additional dependencies?

Fix bug in java versions >9
I think Java is up to double digit versions now. For example, `R CMD javareconf` produces:
Java version: 10.0.2

It looks like line 46:

```
if (jversion < "1.5.0")stop(...)
```

 calls a simple `<` to check versions which does not parse this correctly. For example `"10.0.2" < "1.0.6"` evaluates to `TRUE` in R. It looks like the `utils` package has a `compareVersion` function which seems to evaluate correctly e.g. `utils::compareVersion(jversion,"1.5.0")` returns 1. 

It looks like this package already has dependencies on `utils` so would this be a simple fix without adding any additional dependencies?
@colearendt

This comment has been minimized.

Show comment
Hide comment
@colearendt

colearendt Aug 28, 2018

Collaborator

Looks great! Thanks for the PR! Will evaluate more fully soon.

Collaborator

colearendt commented Aug 28, 2018

Looks great! Thanks for the PR! Will evaluate more fully soon.

@colearendt

This comment has been minimized.

Show comment
Hide comment
@colearendt

colearendt Sep 4, 2018

Collaborator

I think you make a good point that string comparison is less than ideal, here. However, it seems to me that the string comparison is working fine? What locale are you in? Do you mind sharing the output of

Sys.getlocale(); Sys.localeconv();

In my environment:

"1.0.2" < "10.0.6"
#> [1] TRUE
"10.0.2" < "1.0.6"
#> [1] FALSE
"10.0.6" < "1.0.2"
#> [1] FALSE
"1.0" < "10.0.6"
#> [1] TRUE
"1.9" < "10"
#> [1] TRUE
"1.9" < "10.2"
#> [1] TRUE

Created on 2018-09-04 by the reprex
package
(v0.2.0).

Collaborator

colearendt commented Sep 4, 2018

I think you make a good point that string comparison is less than ideal, here. However, it seems to me that the string comparison is working fine? What locale are you in? Do you mind sharing the output of

Sys.getlocale(); Sys.localeconv();

In my environment:

"1.0.2" < "10.0.6"
#> [1] TRUE
"10.0.2" < "1.0.6"
#> [1] FALSE
"10.0.6" < "1.0.2"
#> [1] FALSE
"1.0" < "10.0.6"
#> [1] TRUE
"1.9" < "10"
#> [1] TRUE
"1.9" < "10.2"
#> [1] TRUE

Created on 2018-09-04 by the reprex
package
(v0.2.0).

@sherrillmix

This comment has been minimized.

Show comment
Hide comment
@sherrillmix

sherrillmix Sep 4, 2018

Uhoh looks like we're in tricky string territory. My output looks pretty different (but not completely opposite) from yours:

> "1.0.2" < "10.0.6"
[1] FALSE
> "10.0.2" < "1.0.6"
[1] TRUE
> "10.0.6" < "1.0.2"
[1] TRUE
> "1.0" < "10.0.6"
[1] TRUE
> "1.9" < "10"
[1] FALSE
> "1.9" < "10.2"
[1] FALSE

Sorry should have given my environment on the first post:

>Sys.getlocale(); Sys.localeconv();
[1] "LC_CTYPE=en_US.UTF-8;LC_NUMERIC=C;LC_TIME=en_US.UTF-8;LC_COLLATE=en_US.UTF-8;LC_MONETARY=en_US.UTF-8;LC_MESSAGES=en_US.UTF-8;LC_PAPER=en_US.UTF-8;LC_NAME=C;LC_ADDRESS=C;LC_TELEPHONE=C;LC_MEASUREMENT=en_US.UTF-8;LC_IDENTIFICATION=C"
    decimal_point     thousands_sep          grouping   int_curr_symbol 
              "."                ""                ""            "USD " 
  currency_symbol mon_decimal_point mon_thousands_sep      mon_grouping 
              "$"               "."               ","        "\003\003" 
    positive_sign     negative_sign   int_frac_digits       frac_digits 
               ""               "-"               "2"               "2" 
    p_cs_precedes    p_sep_by_space     n_cs_precedes    n_sep_by_space 
              "1"               "0"               "1"               "0" 
      p_sign_posn       n_sign_posn 
              "1"               "1"

I guess there must be locale specific differences in the sort order of "." vs "[0-9]".

I was curious how utils::compareVersion is doing things. Looks like it's just a strsplit on "[.-]" and integer conversion:

a <- as.integer(strsplit(a, "[.-]")[[1L]])

sherrillmix commented Sep 4, 2018

Uhoh looks like we're in tricky string territory. My output looks pretty different (but not completely opposite) from yours:

> "1.0.2" < "10.0.6"
[1] FALSE
> "10.0.2" < "1.0.6"
[1] TRUE
> "10.0.6" < "1.0.2"
[1] TRUE
> "1.0" < "10.0.6"
[1] TRUE
> "1.9" < "10"
[1] FALSE
> "1.9" < "10.2"
[1] FALSE

Sorry should have given my environment on the first post:

>Sys.getlocale(); Sys.localeconv();
[1] "LC_CTYPE=en_US.UTF-8;LC_NUMERIC=C;LC_TIME=en_US.UTF-8;LC_COLLATE=en_US.UTF-8;LC_MONETARY=en_US.UTF-8;LC_MESSAGES=en_US.UTF-8;LC_PAPER=en_US.UTF-8;LC_NAME=C;LC_ADDRESS=C;LC_TELEPHONE=C;LC_MEASUREMENT=en_US.UTF-8;LC_IDENTIFICATION=C"
    decimal_point     thousands_sep          grouping   int_curr_symbol 
              "."                ""                ""            "USD " 
  currency_symbol mon_decimal_point mon_thousands_sep      mon_grouping 
              "$"               "."               ","        "\003\003" 
    positive_sign     negative_sign   int_frac_digits       frac_digits 
               ""               "-"               "2"               "2" 
    p_cs_precedes    p_sep_by_space     n_cs_precedes    n_sep_by_space 
              "1"               "0"               "1"               "0" 
      p_sign_posn       n_sign_posn 
              "1"               "1"

I guess there must be locale specific differences in the sort order of "." vs "[0-9]".

I was curious how utils::compareVersion is doing things. Looks like it's just a strsplit on "[.-]" and integer conversion:

a <- as.integer(strsplit(a, "[.-]")[[1L]])
@colearendt

This comment has been minimized.

Show comment
Hide comment
@colearendt

colearendt Sep 4, 2018

Collaborator

I was kinda hoping that your locale would look more different from mine than it does! There are a few differences that may be the culprit, though - I don't know enough about the internals of string comparison to say.

Sys.getlocale(); Sys.localeconv()
#> [1] "LC_CTYPE=C.UTF-8;LC_NUMERIC=C;LC_TIME=C.UTF-8;LC_COLLATE=C.UTF-8;LC_MONETARY=C.UTF-8;LC_MESSAGES=C.UTF-8;LC_PAPER=C.UTF-8;LC_NAME=C;LC_ADDRESS=C;LC_TELEPHONE=C;LC_MEASUREMENT=C.UTF-8;LC_IDENTIFICATION=C"
#>     decimal_point     thousands_sep          grouping   int_curr_symbol 
#>               "."                ""                ""                "" 
#>   currency_symbol mon_decimal_point mon_thousands_sep      mon_grouping 
#>                ""               "."                ""                "" 
#>     positive_sign     negative_sign   int_frac_digits       frac_digits 
#>                ""                ""             "127"             "127" 
#>     p_cs_precedes    p_sep_by_space     n_cs_precedes    n_sep_by_space 
#>             "127"             "127"             "127"             "127" 
#>       p_sign_posn       n_sign_posn 
#>             "127"             "127"

Created on 2018-09-04 by the reprex
package
(v0.2.0).

I will say this is definitely a switch worth making, as we do not want failures based on locale. One last check to confirm, can you test in your locale and find a version number that incorrectly fails the last expect_silent test (where we compare to 1.5.0)?

library(testthat)
test_that("version comparison", {
  
  expect_error(with_mock(
    `rJava::.jcall` = function(...){return("1.0.2")}
    , {
      xlsx:::.onLoad(".","xlsx")
    }
  ), "1\\.0\\.2")
  expect_silent(with_mock(
    `rJava::.jcall` = function(...){return("9.0.2")}
    , {
      xlsx:::.onLoad(".","xlsx")
    }
  )
  )
  expect_silent(with_mock(
    `rJava::.jcall` = function(...){return("19.0.2")}
    , {
      xlsx:::.onLoad(".","xlsx")
    }
  ))
})
Collaborator

colearendt commented Sep 4, 2018

I was kinda hoping that your locale would look more different from mine than it does! There are a few differences that may be the culprit, though - I don't know enough about the internals of string comparison to say.

Sys.getlocale(); Sys.localeconv()
#> [1] "LC_CTYPE=C.UTF-8;LC_NUMERIC=C;LC_TIME=C.UTF-8;LC_COLLATE=C.UTF-8;LC_MONETARY=C.UTF-8;LC_MESSAGES=C.UTF-8;LC_PAPER=C.UTF-8;LC_NAME=C;LC_ADDRESS=C;LC_TELEPHONE=C;LC_MEASUREMENT=C.UTF-8;LC_IDENTIFICATION=C"
#>     decimal_point     thousands_sep          grouping   int_curr_symbol 
#>               "."                ""                ""                "" 
#>   currency_symbol mon_decimal_point mon_thousands_sep      mon_grouping 
#>                ""               "."                ""                "" 
#>     positive_sign     negative_sign   int_frac_digits       frac_digits 
#>                ""                ""             "127"             "127" 
#>     p_cs_precedes    p_sep_by_space     n_cs_precedes    n_sep_by_space 
#>             "127"             "127"             "127"             "127" 
#>       p_sign_posn       n_sign_posn 
#>             "127"             "127"

Created on 2018-09-04 by the reprex
package
(v0.2.0).

I will say this is definitely a switch worth making, as we do not want failures based on locale. One last check to confirm, can you test in your locale and find a version number that incorrectly fails the last expect_silent test (where we compare to 1.5.0)?

library(testthat)
test_that("version comparison", {
  
  expect_error(with_mock(
    `rJava::.jcall` = function(...){return("1.0.2")}
    , {
      xlsx:::.onLoad(".","xlsx")
    }
  ), "1\\.0\\.2")
  expect_silent(with_mock(
    `rJava::.jcall` = function(...){return("9.0.2")}
    , {
      xlsx:::.onLoad(".","xlsx")
    }
  )
  )
  expect_silent(with_mock(
    `rJava::.jcall` = function(...){return("19.0.2")}
    , {
      xlsx:::.onLoad(".","xlsx")
    }
  ))
})
@sherrillmix

This comment has been minimized.

Show comment
Hide comment
@sherrillmix

sherrillmix Sep 4, 2018

Oh never knew about with_mock(). That's handy.

Going back to a machine with old 1.8.0_181 Java on it so I can install the package, it seems like decimals are completely ignored when sorting strings in my locale:

> for(ii in 1:106)tryCatch(testthat::with_mock(`rJava::.jcall` = function(...){return(sprintf("%d.0.2",ii))},xlsx:::.onLoad('.','xlsx')),error=function(e)message(e,'\n'))
Error in xlsx:::.onLoad(".", "xlsx"): Your java version is 1.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is 10.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is 11.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is 12.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is 13.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is 14.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is 100.0.2.  Need 1.5.0 or higher.


`Error in xlsx:::.onLoad(".", "xlsx"): Your java version is 101.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is 102.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is 103.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is 104.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is 105.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is 106.0.2.  Need 1.5.0 or higher.

Adding a leading decimal changes nothing:

> for(ii in 1:100)tryCatch(testthat::with_mock(`rJava::.jcall` = function(...){return(sprintf(".%d.0.2",ii))},xlsx:::.onLoad('.','xlsx')),error=function(e)message(sprintf('%s\n',e)))
Error in xlsx:::.onLoad(".", "xlsx"): Your java version is .1.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is .10.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is .11.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is .12.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is .13.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is .14.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is .100.0.2.  Need 1.5.0 or higher.

The problem comes back between 100 to 149 and stop again at 150 and 1000 to 1499:

> for(ii in c(140:160,1498:1501))tryCatch(testthat::with_mock(`rJava::.jcall` = function(...){return(sprintf("%d.0.2",ii))},xlsx:::.onLoad('.','xlsx')),error=function(e)message(sprintf('%s\n',e)))
Error in xlsx:::.onLoad(".", "xlsx"): Your java version is 140.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is 141.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is 142.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is 143.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is 144.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is 145.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is 146.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is 147.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is 148.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is 149.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is 1498.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is 1499.0.2.  Need 1.5.0 or higher.

Running the same on the compareVersion() version comes up clean:

> for(ii in 0:1600)tryCatch(testthat::with_mock(`rJava::.jcall` = function(...){return(sprintf("%d.0.2",ii))},xlsx:::.onLoad('.','xlsx')),error=function(e)message(sprintf('%s\n',e)))
Error in xlsx:::.onLoad(".", "xlsx"): Your java version is 0.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is 1.0.2.  Need 1.5.0 or higher.

Adding a leading decimal place generates an error with compareVersion:

> testthat::with_mock(`rJava::.jcall` = function(...){return(".0.2")},xlsx:::.onLoad('.','xlsx'))
Error in if (a[k] > b[k]) return(1) else if (a[k] < b[k]) return(-1L) : 
  missing value where TRUE/FALSE needed

As do multiple periods in a row:

> testthat::with_mock(`rJava::.jcall` = function(...){return("1....0.2")},xlsx:::.onLoad('.','xlsx'))
Error in if (a[k] > b[k]) return(1) else if (a[k] < b[k]) return(-1L) : 
  missing value where TRUE/FALSE needed

sherrillmix commented Sep 4, 2018

Oh never knew about with_mock(). That's handy.

Going back to a machine with old 1.8.0_181 Java on it so I can install the package, it seems like decimals are completely ignored when sorting strings in my locale:

> for(ii in 1:106)tryCatch(testthat::with_mock(`rJava::.jcall` = function(...){return(sprintf("%d.0.2",ii))},xlsx:::.onLoad('.','xlsx')),error=function(e)message(e,'\n'))
Error in xlsx:::.onLoad(".", "xlsx"): Your java version is 1.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is 10.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is 11.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is 12.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is 13.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is 14.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is 100.0.2.  Need 1.5.0 or higher.


`Error in xlsx:::.onLoad(".", "xlsx"): Your java version is 101.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is 102.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is 103.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is 104.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is 105.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is 106.0.2.  Need 1.5.0 or higher.

Adding a leading decimal changes nothing:

> for(ii in 1:100)tryCatch(testthat::with_mock(`rJava::.jcall` = function(...){return(sprintf(".%d.0.2",ii))},xlsx:::.onLoad('.','xlsx')),error=function(e)message(sprintf('%s\n',e)))
Error in xlsx:::.onLoad(".", "xlsx"): Your java version is .1.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is .10.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is .11.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is .12.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is .13.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is .14.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is .100.0.2.  Need 1.5.0 or higher.

The problem comes back between 100 to 149 and stop again at 150 and 1000 to 1499:

> for(ii in c(140:160,1498:1501))tryCatch(testthat::with_mock(`rJava::.jcall` = function(...){return(sprintf("%d.0.2",ii))},xlsx:::.onLoad('.','xlsx')),error=function(e)message(sprintf('%s\n',e)))
Error in xlsx:::.onLoad(".", "xlsx"): Your java version is 140.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is 141.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is 142.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is 143.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is 144.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is 145.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is 146.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is 147.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is 148.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is 149.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is 1498.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is 1499.0.2.  Need 1.5.0 or higher.

Running the same on the compareVersion() version comes up clean:

> for(ii in 0:1600)tryCatch(testthat::with_mock(`rJava::.jcall` = function(...){return(sprintf("%d.0.2",ii))},xlsx:::.onLoad('.','xlsx')),error=function(e)message(sprintf('%s\n',e)))
Error in xlsx:::.onLoad(".", "xlsx"): Your java version is 0.0.2.  Need 1.5.0 or higher.


Error in xlsx:::.onLoad(".", "xlsx"): Your java version is 1.0.2.  Need 1.5.0 or higher.

Adding a leading decimal place generates an error with compareVersion:

> testthat::with_mock(`rJava::.jcall` = function(...){return(".0.2")},xlsx:::.onLoad('.','xlsx'))
Error in if (a[k] > b[k]) return(1) else if (a[k] < b[k]) return(-1L) : 
  missing value where TRUE/FALSE needed

As do multiple periods in a row:

> testthat::with_mock(`rJava::.jcall` = function(...){return("1....0.2")},xlsx:::.onLoad('.','xlsx'))
Error in if (a[k] > b[k]) return(1) else if (a[k] < b[k]) return(-1L) : 
  missing value where TRUE/FALSE needed
@sherrillmix

This comment has been minimized.

Show comment
Hide comment
@sherrillmix

sherrillmix Sep 4, 2018

Also it appears that is probably the LC_COLLATE option of our locales causing differences (mine is en_US.UTF-8 and yours is C.UTF-8) with similar problems coming up frequently.

sherrillmix commented Sep 4, 2018

Also it appears that is probably the LC_COLLATE option of our locales causing differences (mine is en_US.UTF-8 and yours is C.UTF-8) with similar problems coming up frequently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment