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

vulnerability to PROJ6/GDAL3 #18

Closed
rsbivand opened this issue Jan 9, 2020 · 6 comments
Closed

vulnerability to PROJ6/GDAL3 #18

rsbivand opened this issue Jan 9, 2020 · 6 comments

Comments

@rsbivand
Copy link

rsbivand commented Jan 9, 2020

Running revdeps from sp (sp (my github fork) with development rgdal from R-Forge):

> packageVersion("sp")
[1] ‘1.3.4’
> packageVersion("rgdal")
[1] ‘1.5.3’
> rgdal::rgdal_extSoftVersion()
          GDAL GDAL_with_GEOS           PROJ             sp 
       "3.0.2"         "TRUE"        "6.2.1"        "1.3-2" 

a test fails:

> library(testthat)
> library(BIOMASS)
> 
> test_check("BIOMASS")
── 1. Failure: ComputeAGB with coord (@test_03_computeAGB.R#41)  ───────────────
`computeAGB(D, WD$meanWD, coord = c(74.91944, 14.36806))` produced warnings.

══ testthat results  ═══════════════════════════════════════════════════════════
[ OK: 472 | SKIPPED: 0 | WARNINGS: 81 | FAILED: 1 ]
1. Failure: ComputeAGB with coord (@test_03_computeAGB.R#41) 

Error: testthat unit tests failed
Execution halted

See:

http://rgdal.r-forge.r-project.org/articles/PROJ6_GDAL3.html
r-spatial/sf#1231
r-spatial/sf#1187
r-spatial/sf#1146
r-spatial/discuss#28

for background. See:

r-spatial/discuss#28 (comment)

for a way of testing fixes in a docker container contributed by Jakub Nowosad.

@rsbivand
Copy link
Author

rsbivand commented Mar 31, 2020

The development version of rgdal will be released soon, so now is the time to correct this. Please act now, or be obliged to act when CRAN gives you a short deadline to avoid being archived. computeAGB(D, WD$meanWD, coord = c(74.91944, 14.36806)) fails in some way; you are making unfounded assumptions with PROJ6/GDAL3. At the very least, comment out these tests until you understand what has changed, and release ASAP after testing that the revised package no longer fails with rgdal > 1.5 from R-Forge; the modernised sp is already on CRAN. If you think the problem is in raster, raise an issue there and comment out the test. There are 50 packages not responding to warnings, I cannot resolve this for you.

@rsbivand
Copy link
Author

Action is required urgently. The problem is the line specified:

expect_silent(computeAGB(D, WD$meanWD, coord = c(74.91944, 14.36806)))

which should simply not abuse the developers of rgdal and raster by assuming that they will never wish to alert you to possible workflow degradations. Running with debug() for computeAGD() and computeE(),

RAST <- raster(cacheManager("E"))
Warning message:
In showSRID(uprojargs, format = "PROJ", multiline = "NO") :
  Discarded datum Unknown based on WGS84 ellipsoid in CRS definition

and

rgdal::GDALinfo(cacheManager("E"))
rows        3600 
columns     8640 
bands       1 
lower left origin.x        -180 
lower left origin.y        -60 
res.x       0.04166667 
res.y       0.04166667 
ysign       -1 
oblique.x   0 
oblique.y   0 
driver      EHdr 
projection  +proj=longlat +ellps=WGS84 +no_defs 
file        /home/rsb/.local/share/R/BIOMASS/E/E.bil 
apparent band summary:
   GDType hasNoDataValue NoDataValue blockSize1 blockSize2
1 Float32           TRUE   -1.7e+308          1       8640
apparent band statistics:
         Bmin       Bmax Bmean Bsd
1 -4294967295 4294967295    NA  NA
Warning messages:
1: In getProjectionRef(x, OVERRIDE_PROJ_DATUM_WITH_TOWGS84 = OVERRIDE_PROJ_DATUM_WITH_TOWGS84,  :
  Discarded datum D_unknown in CRS definition: +proj=longlat +ellps=WGS84 +no_defs
2: In rgdal::GDALinfo(cacheManager("E")) :
  statistics not supported by this driver

So you need to decide how to handle degradations in coordinate reference systems, see also: https://rgdal.r-forge.r-project.org/articles/CRS_projections_transformations.html

rgdal will be submitted to CRAN very shortly.

@rsbivand
Copy link
Author

rsbivand commented Jun 24, 2020

This leads to an error in your 2.1.2 in one platform on CRAN which will spread - mute/drop expect_silent(computeAGB(D, WD$meanWD, coord = c(74.91944, 14.36806))) immediately. I'm seeing this running the development version of raster. Never, ever use testthat::expect_silent() unless you control all of the computations.

@MaximeRM
Copy link
Collaborator

MaximeRM commented Jun 25, 2020 via email

@rsbivand
Copy link
Author

When that environment variable is set: `R_CHECK_SUGGESTS_ONLY=false R CMD check --as-cran BIOMASS_2.1.2.tar.gz on a system without any of the Suggests: packages, the package should pass. Prepare a platform first with only the Depends: and Imports: packages, installed with dependencies=NA, so that they recursively install only packages that they need but do not just suggest. Some of your Suggests: will be present, but others will not. The package must check that Suggests: packages can be loaded/attached before they are used. When you find the error caused by a missing suggested package, you'll also find the place in your examples, tests, vignettes or code that must be protected from execution unless the package is present on the system. Package installation only ever installs Depends:, Imports: and Linking-To: unless told to install Suggests too.

@MaximeRM
Copy link
Collaborator

MaximeRM commented Jun 25, 2020 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants