r-quantities / units

Measurement units for R
https://r-quantities.github.io/units
173 stars 27 forks source link

install_unit() that is equivalent to an existing unit - different behaviour on Windows #301

Closed ateucher closed 1 year ago

ateucher commented 2 years ago

I would like to be able to install a unit that is equivalent (identical) to an already installed unit. This works fine on macOS and Linux, but fails on Windows

Using the latest version of {units} - I've confirmed this with on macOS with the binary CRAN version and the latest github version (built with the latest {udunits2} from homebrew), but it fails on Windows.

macOS

library(units)
#> udunits database from /usr/local/share/udunits/udunits2.xml

install_unit("foobar", "1 kg", "A foobar")
x <- as_units(5, "foobar")
x
#> 5 [foobar]
units(x) <- "g"
x
#> 5000 [g]

Windows

units::install_unit("foobar", "1 kg", "A foobar")
#> Error in ud_map_symbols(symbol, ut_unit) : Unit already maps to "kg"
#> Backtrace:
#> x
#> 1. \-units::install_unit("foobar", "1 kg", "A foobar")
#> 2. \-units:::ud_map_symbols(symbol, ut_unit)

I notice that {units} on Windows is built against an old version of udunits2 (v2.2.20), whereas it is built against the current (v2.2.28) version on macOS, so I'm guessing the difference might lie there?

Enchufa2 commented 2 years ago

Can you provide please the sessionInfo for both systems?

ateucher commented 2 years ago

Here it is for my Mac, my colleague @boshek will post his for Windows...

R version 4.1.2 (2021-11-01)
Platform: x86_64-apple-darwin17.0 (64-bit)
Running under: macOS Big Sur 11.6.3

Matrix products: default
LAPACK: /Library/Frameworks/R.framework/Versions/4.1/Resources/lib/libRlapack.dylib

locale:
[1] en_CA.UTF-8/en_CA.UTF-8/en_CA.UTF-8/C/en_CA.UTF-8/en_CA.UTF-8

attached base packages:
[1] stats     graphics  grDevices utils     datasets  methods   base     

other attached packages:
[1] units_0.8-0

loaded via a namespace (and not attached):
[1] compiler_4.1.2 cli_3.1.1      tools_4.1.2    Rcpp_1.0.8     rlang_1.0.1
boshek commented 2 years ago

For the windows system:

R> sessioninfo::session_info()
- Session info -------------------------------------------------------------------------------------------------
 setting  value
 version  R version 4.1.2 (2021-11-01)
 os       Windows 10 x64 (build 19043)
 system   x86_64, mingw32
 ui       RStudio
 language (EN)
 collate  English_Canada.1252
 ctype    English_Canada.1252
 tz       America/Los_Angeles
 date     2022-02-10
 rstudio  1.4.1717 Juliet Rose (desktop)
 pandoc   2.11.4 @ C:\\PROGRA~1\\RStudio\\bin\\pandoc\\pandoc.exe

- Packages -----------------------------------------------------------------------------------------------------
 package     * version date (UTC) lib source
 cli           3.1.1   2022-01-20 [1] CRAN (R 4.1.2)
 crayon        1.4.2   2021-10-29 [1] CRAN (R 4.1.1)
 ellipsis      0.3.2   2021-04-29 [1] CRAN (R 4.0.5)
 fansi         1.0.2   2022-01-14 [1] CRAN (R 4.1.2)
 glue          1.6.1   2022-01-22 [1] CRAN (R 4.1.2)
 httr          1.4.2   2020-07-20 [1] CRAN (R 4.0.2)
 lifecycle     1.0.1   2021-09-24 [1] CRAN (R 4.1.0)
 magrittr      2.0.2   2022-01-26 [1] CRAN (R 4.1.2)
 pillar        1.7.0   2022-02-01 [1] CRAN (R 4.1.2)
 pkgconfig     2.0.3   2019-09-22 [1] CRAN (R 4.0.0)
 R6            2.5.1   2021-08-19 [1] CRAN (R 4.1.0)
 Rcpp          1.0.8   2022-01-13 [1] CRAN (R 4.1.2)
 rlang         1.0.0   2022-01-26 [1] CRAN (R 4.1.2)
 rstudioapi    0.13    2020-11-12 [1] CRAN (R 4.0.3)
 rvest         1.0.2   2021-10-16 [1] CRAN (R 4.1.1)
 sessioninfo   1.2.2   2021-12-06 [1] CRAN (R 4.1.2)
 tibble        3.1.6   2021-11-07 [1] CRAN (R 4.1.2)
 units       * 0.8-0   2022-02-10 [1] Github (r-quantities/units@ff6b8ae)
 utf8          1.2.2   2021-07-24 [1] CRAN (R 4.1.0)
 vctrs         0.3.8   2021-04-29 [1] CRAN (R 4.0.5)
 xml2          1.3.3   2021-11-30 [1] CRAN (R 4.1.2)

 [1] C:/Users/salbers/R/win-library/4.0
 [2] C:/Users/salbers/R/win-library/4.1
 [3] C:/Program Files/R/R-4.1.2/library

----------------------------------------------------------------------------------------------------------------
Enchufa2 commented 2 years ago

Install the latest version from CRAN, please.

boshek commented 2 years ago

Same result

units::install_unit("foobar", "1 kg", "A foobar")
#> Error in ud_map_symbols(symbol, ut_unit): Unit already maps to "kg"
Session info ``` r sessioninfo::session_info() #> - Session info --------------------------------------------------------------- #> setting value #> version R version 4.1.2 (2021-11-01) #> os Windows 10 x64 (build 19043) #> system x86_64, mingw32 #> ui RTerm #> language (EN) #> collate English_Canada.1252 #> ctype English_Canada.1252 #> tz America/Los_Angeles #> date 2022-02-10 #> pandoc 2.11.4 @ C:/Program Files/RStudio/bin/pandoc/ (via rmarkdown) #> #> - Packages ------------------------------------------------------------------- #> package * version date (UTC) lib source #> backports 1.4.1 2021-12-13 [1] CRAN (R 4.1.2) #> cli 3.1.1 2022-01-20 [1] CRAN (R 4.1.2) #> crayon 1.4.2 2021-10-29 [1] CRAN (R 4.1.1) #> digest 0.6.29 2021-12-01 [1] CRAN (R 4.1.2) #> ellipsis 0.3.2 2021-04-29 [1] CRAN (R 4.0.5) #> evaluate 0.14 2019-05-28 [1] CRAN (R 4.0.0) #> fansi 1.0.2 2022-01-14 [1] CRAN (R 4.1.2) #> fastmap 1.1.0 2021-01-25 [1] CRAN (R 4.0.3) #> fs 1.5.2 2021-12-08 [1] CRAN (R 4.1.2) #> glue 1.6.1 2022-01-22 [1] CRAN (R 4.1.2) #> highr 0.9 2021-04-16 [1] CRAN (R 4.0.4) #> htmltools 0.5.2 2021-08-25 [1] CRAN (R 4.1.1) #> knitr 1.37 2021-12-16 [1] CRAN (R 4.1.2) #> lifecycle 1.0.1 2021-09-24 [1] CRAN (R 4.1.0) #> magrittr 2.0.2 2022-01-26 [1] CRAN (R 4.1.2) #> pillar 1.7.0 2022-02-01 [1] CRAN (R 4.1.2) #> pkgconfig 2.0.3 2019-09-22 [1] CRAN (R 4.0.0) #> purrr 0.3.4 2020-04-17 [1] CRAN (R 4.0.0) #> R.cache 0.15.0 2021-04-30 [1] CRAN (R 4.0.5) #> R.methodsS3 1.8.1 2020-08-26 [1] CRAN (R 4.0.2) #> R.oo 1.24.0 2020-08-26 [1] CRAN (R 4.0.2) #> R.utils 2.11.0 2021-09-26 [1] CRAN (R 4.1.0) #> Rcpp 1.0.8 2022-01-13 [1] CRAN (R 4.1.2) #> reprex 2.0.1 2021-08-05 [1] CRAN (R 4.1.0) #> rlang 1.0.0 2022-01-26 [1] CRAN (R 4.1.2) #> rmarkdown 2.11 2021-09-14 [1] CRAN (R 4.1.0) #> rstudioapi 0.13 2020-11-12 [1] CRAN (R 4.0.3) #> sessioninfo 1.2.2 2021-12-06 [1] CRAN (R 4.1.2) #> stringi 1.7.6 2021-11-29 [1] CRAN (R 4.1.2) #> stringr 1.4.0 2019-02-10 [1] CRAN (R 4.0.2) #> styler 1.6.2 2021-09-23 [1] CRAN (R 4.1.1) #> tibble 3.1.6 2021-11-07 [1] CRAN (R 4.1.2) #> units 0.8-0 2022-02-05 [1] CRAN (R 4.1.2) #> utf8 1.2.2 2021-07-24 [1] CRAN (R 4.1.0) #> vctrs 0.3.8 2021-04-29 [1] CRAN (R 4.0.5) #> withr 2.4.3 2021-11-30 [1] CRAN (R 4.1.0) #> xfun 0.29 2021-12-14 [1] CRAN (R 4.1.2) #> yaml 2.2.2 2022-01-25 [1] CRAN (R 4.1.2) #> #> [1] C:/Users/salbers/R/win-library/4.0 #> [2] C:/Users/salbers/R/win-library/4.1 #> [3] C:/Program Files/R/R-4.1.2/library #> #> ------------------------------------------------------------------------------ ```
ateucher commented 2 years ago

I've forked the repo and created a test for this; It fails on Windows (as reported here), but passes on all other platforms: https://github.com/ateucher/units/runs/5149759655?check_suite_focus=true

Enchufa2 commented 2 years ago

Thanks, @ateucher, that's very useful. The only difference I see is the version of the library. udunits v2.2.20 dates October 2015. The only thing we can do is to ask CRAN to update it.

I was trying to see what changed in the library, but it's not obvious.

Enchufa2 commented 2 years ago

Nope, I installed v2.2.20 on Linux and it works just fine. The library does some things differently on Windows, so this is an upstream bug worth reporting.

ateucher commented 2 years ago

That is interesting... though I feel like there's a chance that it did get fixed upstream in the updates between v2.2.20 and v2.2.28, so it may be worth asking if we can get the Windows udunits2 library updated regardless? I can ping @jeroen to see if we can update this, which as far as I can tell is what is used to build the package on Windows (https://github.com/r-quantities/units/blob/main/src/Makevars.win)? Or does CRAN use a different source of udunits2 to build binaries?

Enchufa2 commented 2 years ago

That is interesting... though I feel like there's a chance that it did get fixed upstream in the updates between v2.2.20 and v2.2.28

I don't think so. There's nothing even related in the changelog, and no significant changes in the functions involved. However, they use different code paths to search for units in Windows and everywhere else (see here). So I believe there's a bug in the Windows code path.

so it may be worth asking if we can get the Windows udunits2 library updated regardless?

2015 was... a long time ago. :) So yes, we could all benefit from an update regardless of this issue.

I can ping @jeroen to see if we can update this, which as far as I can tell is what is used to build the package on Windows? Or does CRAN use a different source of udunits2 to build binaries?

AFAIK, CRAN uses a separate toolchain for their checks, but Jeroen maintains rtools4? Not sure. And MSYS2 seems to have a newer version. So... I'm a bit lost here.

jeroen commented 2 years ago

I will update the builds in rwinlib/udunits. There is also a newer version in the https://github.com/rwinlib/gdal3 repo, but that is maybe a bit overkill.

ateucher commented 2 years ago

Thanks so much @jeroen! Are you (or @edzer?) able to shed some light on whether CRAN builds its Windows binaries with the udunits that is in MSYS2, or with that in rwinlib/udunits? I feel like the latter given the Makevars.win which calls this script, pulling from rwinlib/udunits? But CRAN's build processes are a bit opaque to me so I feel like there's a good chance I'm wrong...

jeroen commented 2 years ago

For R 4.0 and 4.1 we download the version from rwinlib. For R 4.2, CRAN uses their own local copy.

Enchufa2 commented 2 years ago

@ateucher As I suspected, v2.2.28 doesn't solve the issue. Feel free to escalate this upstream, but in my experience, unfortunately they are not very responsive.

jeroen commented 2 years ago

Maybe it has to do with the version of the 'share' files. Let me try something.

jeroen commented 2 years ago

No that wasn't the issue, never mind :)

ateucher commented 2 years ago

Ah, that is unfortunate! Thanks so much for your help @jeroen and @Enchufa2

Enchufa2 commented 2 years ago

Reported upstream. It would be nice if @boshek could please compile the small example I put there on Windows to double check it fails.

ateucher commented 2 years ago

So I gave this a shot on a Windows machine... compiling C/C++ is still pretty new to me, but this is what I did:

  1. Set up a msys2 system using Rtools4.0
  2. Installed udunits2 with pacman (this is the same library used the build the units package):
    pacman -Sy
    pacman -S mingw-w64-x86_64-udunits
  3. copied your example (slightly modified for the header location) to a file called test-units.c:
    
    #include <stdio.h>
    #include <udunits2.h>

int main() { ut_set_error_message_handler((ut_error_message_handler) ut_ignore); ut_system *sys = ut_read_xml(NULL); ut_encoding enc = UT_UTF8; ut_set_error_message_handler((ut_error_message_handler) vprintf);

ut_unit *m = ut_parse(sys, "1 m", enc);
ut_map_symbol_to_unit("my_metre", enc, m);
// fails on Windows, works fine on Unix
ut_map_unit_to_symbol(m, "my_metre", enc);
// Error: Unit already maps to "m"

ut_free(m);
ut_free_system(sys);
return 0;

}

4. After a bit of experimenting and getting lots of errors locating the correct libraries etc. ran this:

gcc test-units.c -l udunits2 -l expat -IC:/rtools40/mingw64/include/


... and lo and behold, it ran with no error (returned exit code `0`). So I don't know what's up now @Enchufa2 - any thoughts?
ateucher commented 2 years ago

PS - For completeness I did this on my Mac and it was also fine...

Enchufa2 commented 2 years ago

Sorry, I missed this. I'm a bit lost, because we have now a failure on Debian too, so I'm not sure what causes the issue anymore... :( If I'm not able to reproduce this, then I cannot debug it.

Enchufa2 commented 1 year ago

So... not only we didn't manage to reproduce this consistently, but now this now runs on Windows just fine too. :(

Closing here. But please feel free to reopen if you find the issue again.