zoonproject / zoon

The zoon R package
Other
61 stars 13 forks source link

vulnerability to GDAL3/PROJ6+ #426

Closed rsbivand closed 4 years ago

rsbivand commented 4 years ago

In running revdep checks with the development version of rgdal from R-Forge install.packages("rgdal", repos="http://R-Forge.R-project.org") with GDAL 3.0.4 and PROJ 7.0.0 (https://www.r-spatial.org/r/2020/03/17/wkt.html), I am seeing unspecified errors::

* checking tests ...
  Running ‘test-all.R’
 ERROR
Running the tests in ‘tests/test-all.R’ failed.
Last 13 lines of output:
  Stopping workflow due to error in covariate module. 
  Backtrace:
   1. zoon::workflow(...)
   2. base::tryCatch(...)
   3. base:::tryCatchList(expr, classes, parentenv, handlers)
   4. base:::tryCatchOne(expr, names, parentenv, handlers[[1L]])
   5. value[[3L]](cond)
   6. zoon:::ErrorModule(cond, 2, e)

  ══ testthat results  ═══════════════════════════════════════════════════════════
  [ OK: 204 | SKIPPED: 31 | WARNINGS: 128 | FAILED: 1 ]
  1. Error: Test in workflows (@testCombineRasters.R#94) 

  Error: testthat unit tests failed
  Execution halted

Since rgdal will be submitted to CRAN soon, it would be sensible to establish whether this error is related to necessary changes in rgdal, and how to address the problems.

AugustT commented 4 years ago

When are you planning on submitting to CRAN?

rsbivand commented 4 years ago

Currently working through dozens of similar cases, maybe next week if I get lucky. R-Forge will track my progress in finding ways to mitigate the too optimistic coding assumptions of packages depending on rgdal and raster.

AugustT commented 4 years ago

re-ran tests using rgdal on Rforge, while I have not yet been able to replicate the error I get a staggering 521 warnings!

These include things like: Discarded datum Unknown based on WGS84 ellipsoid in CRS definition Discarded datum Unknown based on Airy 1830 ellipsoid in CRS definition Discarded datum OSGB_1936 in CRS definition Using PROJ not WKT2 strings Discarded datum Unknown based on WGS84 ellipsoid in CRS definition, but +towgs84= values preserved

rsbivand commented 4 years ago

Good, they got your attention then? That is what they are for, to alert users to the need to adapt their workflows to avoid PROJ strings wherever possible. They are not CRAN warnings, they are there for a purpose. They can be partially muted by using rgdal::set_thin_PROJ6_warnings(TRUE) in each example section or test file after loading rgdal; then only one warning is issued per script, and a count is kept instead. Each time exportToProj4() in GDAL is used, the difference between the output PROJ string and the internal representation from which it was extracted are compared, and discarded key-value pairs reported. If the workflow migrates to use WKT2 rather than PROJ strings, we can suppress the warnings, or the package maintainer can do so with supressWarnings(), but I think users need to know; otherwise they will think that silence means business as usual, unless they have highly developed checking in place to detect output changes on package update.

AugustT commented 4 years ago

Thanks for the details I will not suppress them but may use 'set_thin_PROJ6_warnings' in tests

I have not been able to replicate your error.

Have you made any changes to your package since the original message that could explain this? If not, I have been testing using a vanilla installation of R so I assumed installing rgdal from R forge would update gdal and proj, but is there a way I can check this?

rsbivand commented 4 years ago

No, rgdal uses the PROJ and GDAL on your platform.

I'm going through revdeps on Windows with the binary packages mentioned here: https://stat.ethz.ch/pipermail/r-sig-geo/2020-April/028063.html. zoon 0.6-5 does not fail for these binary builds of rgdal, PROJ and GDAL.

AugustT commented 4 years ago

Thanks Roger. I have installed the binaries you linked to and can confirm there are no errors in the zoon package during tests. Can we safely assume this issue as gone away because of some change you have made? If so I will close this issue.

rsbivand commented 4 years ago

Either changes in my code, or possibly in raster, as the current version was released April 19. So yes, things look OK at the moment.