Merge pull request #172 from ropensci/gdal-version-check

njtierney · web-flow · commit 00f339cb35f7 · 2025-05-15T16:24:41.000+10:00
Fix for CRAN error relating to GDAL version
diff --git a/.github/workflows/R-CMD-check.yaml b/.github/workflows/R-CMD-check.yaml
@@ -23,6 +23,7 @@ jobs:
           - {os: ubuntu-latest,   r: 'devel', http-user-agent: 'release'}
           - {os: ubuntu-latest,   r: 'release'}
           - {os: ubuntu-latest,   r: 'oldrel-1'}
+          - {os: ubuntu-22.04,    r: 'release'} #test with older version of GDAL
 
     env:
       GITHUB_PAT: ${{ secrets.GITHUB_TOKEN }}
diff --git a/.gitignore b/.gitignore
@@ -16,3 +16,4 @@ docs
 inst/doc
 /doc/
 /Meta/
+.Renviron
diff --git a/DESCRIPTION b/DESCRIPTION
@@ -1,6 +1,6 @@
 Package: geotargets
 Title: 'targets' Extensions for Geographic Spatial Formats
-Version: 0.3.0.9000
+Version: 0.3.1
 Authors@R: c(
     person(
       given = "Nicholas", 
@@ -37,7 +37,8 @@ Imports:
     terra (>= 1.8-10),
     withr (>= 3.0.0),
     zip,
-    lifecycle
+    lifecycle,
+    gdalraster (>= 2.0.0)
 Suggests: 
     crew (>= 0.9.2),
     knitr,
@@ -47,7 +48,6 @@ Suggests:
     stars,
     testthat (>= 3.0.0),
     fs,
-    gdalraster,
     spelling
 Config/testthat/edition: 3
 URL: https://github.com/ropensci/geotargets, https://docs.ropensci.org/geotargets/
diff --git a/NEWS.md b/NEWS.md
@@ -1,4 +1,7 @@
-# geotargets (development version)
+# geotargets 0.3.1 (15 May 2025)
+
+* Throws an error if `preserve_metadata = "gdalraster_sozip"` in function `tar_terra_rast()` and if GDAL is less than 3.7. Skips testing this feature when GDAL < 3.7 also. This fixes a bug picked up by the CRAN team.
+* Tests also don't report progress bars, as mentioned by CRAN team.
 
 # geotargets 0.3.0 (16 April 2025)
 
diff --git a/R/tar-terra-rast.R b/R/tar-terra-rast.R
@@ -113,6 +113,7 @@ tar_terra_rast <- function(
 
   if (preserve_metadata == "gdalraster_sozip") {
     check_pkg_installed("gdalraster")
+    check_gdal_sozip()
   }
 
   # ensure that user-passed `resources` doesn't include `custom_format`
diff --git a/R/utils.R b/R/utils.R
@@ -16,9 +16,29 @@ check_pkg_installed <- function(pkg, call = rlang::caller_env()) {
 }
 
 gdal_version <- function() {
-  numeric_version(terra::gdal(lib = "gdal"))
+  numeric_version(gdalraster::gdal_version()[4])
 }
 
+gdal_version_lt <- function(version) {
+  gdal_version() < numeric_version(version)
+}
+
+
+check_gdal_sozip <- function(call = rlang::caller_env()) {
+  gdal_cannot_sozip <- gdal_version_lt("3.7.0")
+  if (gdal_cannot_sozip) {
+    cli::cli_abort(
+      message = c(
+        "Must have {.pkg GDAL} version {.val 3.7} or greater",
+        "i" = "To use {.arg preserve_metadata = 'gdalraster_sozip'}",
+        "x" = "We see {.pkg GDAL} version: {.val {gdal_version()}}"
+      ),
+      call = call
+    )
+  }
+}
+
+
 check_gdal_shz <- function(min_version = "3.1", call = rlang::caller_env()) {
   if (gdal_version() < numeric_version(min_version)) {
     cli::cli_abort(
diff --git a/tests/testthat/_snaps/check-gdal-sozip.md b/tests/testthat/_snaps/check-gdal-sozip.md
@@ -0,0 +1,10 @@
+# GDAL version checked checked
+
+    Code
+      check_gdal_sozip()
+    Condition
+      Error:
+      ! Must have GDAL version "3.7" or greater
+      i To use `preserve_metadata = 'gdalraster_sozip'`
+      x We see GDAL version: 3.0.0
+
diff --git a/tests/testthat/test-check-gdal-sozip.R b/tests/testthat/test-check-gdal-sozip.R
@@ -0,0 +1,9 @@
+test_that("GDAL version checked checked", {
+  local_mocked_bindings(
+    gdal_version = function(...) numeric_version("3.0.0")
+  )
+  expect_snapshot(
+    error = TRUE,
+    check_gdal_sozip()
+  )
+})
diff --git a/tests/testthat/test-geotargets-option.R b/tests/testthat/test-geotargets-option.R
@@ -11,7 +11,7 @@ targets::tar_test("geotargets_options_get() retrieves options in correct priorit
           )
         )
       })
-      targets::tar_make()
+      targets::tar_make(reporter = "silent")
       expect_identical(
         geotargets::geotargets_option_get("gdal.raster.driver"),
         "GIF"
diff --git a/tests/testthat/test-tar-stars.R b/tests/testthat/test-tar-stars.R
@@ -14,7 +14,7 @@ targets::tar_test("tar_stars() works", {
       )
     ))
   })
-  targets::tar_make()
+  targets::tar_make(reporter = "silent")
   x <- targets::tar_read(test_stars)
   expect_s3_class(x, "stars")
   expect_snapshot(
@@ -34,7 +34,7 @@ targets::tar_test("tar_stars_proxy() works", {
       )
     ))
   })
-  targets::tar_make()
+  targets::tar_make(reporter = "silent")
   x <- targets::tar_read(test_stars_proxy)
   expect_s3_class(x, "stars_proxy")
   expect_snapshot(
@@ -62,7 +62,7 @@ targets::tar_test("tar_stars(mdim=TRUE) works", {
     ))
   })
 
-  targets::tar_make()
+  targets::tar_make(reporter = "silent")
   x <- targets::tar_read(test_stars_mdim)
   expect_s3_class(x, "stars")
   expect_snapshot(x)
@@ -91,7 +91,7 @@ targets::tar_test("tar_stars(mdim=TRUE, ncdf=TRUE) works", {
     ))
   })
 
-  targets::tar_make()
+  targets::tar_make(reporter = "silent")
   # warnings related to no CRS
   suppressWarnings({
     x <- targets::tar_read(test_stars_mdim_ncdf)
@@ -120,6 +120,6 @@ targets::tar_test("tar_stars() works with dynamic branching", {
       )
     )
   })
-  targets::tar_make()
+  targets::tar_make(reporter = "silent")
   expect_length(targets::tar_read(test_stars_plus), 2)
 })
diff --git a/tests/testthat/test-tar-terra-rast.R b/tests/testthat/test-tar-terra-rast.R
@@ -10,7 +10,7 @@ targets::tar_test("tar_terra_rast() works", {
       )
     )
   })
-  targets::tar_make()
+  targets::tar_make(reporter = "silent")
   x <- targets::tar_read(test_terra_rast)
   expect_s4_class(x, "SpatRaster")
   expect_identical(terra::datatype(x), "INT4S")
@@ -35,7 +35,7 @@ targets::tar_test("tar_terra_rast() works with multiple workers (tests un/marsha
       )
     )
   })
-  targets::tar_make()
+  targets::tar_make(reporter = "silent")
   expect_true(all(is.na(targets::tar_meta()$error)))
   expect_s4_class(targets::tar_read(rast1), "SpatRaster")
 })
@@ -58,7 +58,7 @@ targets::tar_test("tar_terra_rast() works with dynamic branching", {
       )
     )
   })
-  targets::tar_make()
+  targets::tar_make(reporter = "silent")
   expect_length(targets::tar_read(my_map_plus), 2)
 })
 
@@ -123,7 +123,7 @@ tar_test("That changing filetype invalidates a target", {
       )
     )
   })
-  tar_make()
+  tar_make(reporter = "silent")
 
   targets::tar_script({
     library(targets)
@@ -163,7 +163,7 @@ tar_test("metadata is maintained for GTiff", {
       tar_terra_rast(r2, make_rast(), preserve_metadata = "drop")
     )
   })
-  tar_make()
+  tar_make(reporter = "silent")
   x <- tar_read(r)
   expect_identical(terra::units(x), rep("m", 3))
   expect_identical(terra::time(x), as.Date("2024-10-01") + c(0, 1, 2))
@@ -192,7 +192,7 @@ tar_test("metadata is maintained for COG", {
       tar_terra_rast(r2, make_rast(), preserve_metadata = "drop")
     )
   })
-  tar_make()
+  tar_make(reporter = "silent")
   x <- tar_read(r)
   expect_identical(terra::units(x), rep("m", 3))
   expect_identical(terra::time(x), as.Date("2024-10-01") + c(0, 1, 2))
@@ -201,6 +201,7 @@ tar_test("metadata is maintained for COG", {
 
 tar_test("metadata is maintained (gdalraster SOZip)", {
   skip_if_not_installed("gdalraster")
+  skip_if_not(gdal_version() >= numeric_version("3.7"))
   tar_script({
     library(targets)
     library(geotargets)
@@ -220,20 +221,24 @@ tar_test("metadata is maintained (gdalraster SOZip)", {
       tar_terra_rast(r, make_rast(), filetype = "HFA")
     )
   })
-  tar_make()
+  tar_make(reporter = "silent")
   x <- tar_read(r)
   expect_identical(terra::units(x), rep("m", 3))
   expect_identical(terra::time(x), as.Date("2024-10-01") + c(0, 1, 2))
-  expect_identical(terra::metags(x), data.frame(
+  expect_identical(
+    terra::metags(x),
+    data.frame(
       name = c("AREA_OR_POINT", "FOO"),
       value = c("Area", "BAR"),
       domain = c("", "")
-  ))
-  expect_identical(terra::metags(x, 1),
-                 data.frame(
-                     layer = 1,
-                     name = "asdf",
-                     value = "hjkl"
-                 )
-)
+    )
+  )
+  expect_identical(
+    terra::metags(x, 1),
+    data.frame(
+      layer = 1,
+      name = "asdf",
+      value = "hjkl"
+    )
+  )
 })
diff --git a/tests/testthat/test-tar-terra-sprc.R b/tests/testthat/test-tar-terra-sprc.R
@@ -37,7 +37,7 @@ targets::tar_test("tar_terra_sprc() works", {
       )
     )
   })
-  targets::tar_make()
+  targets::tar_make(reporter = "silent")
   x <- targets::tar_read(raster_elevs)
   expect_s4_class(x, "SpatRasterCollection")
   expect_snapshot(x)
@@ -82,7 +82,7 @@ targets::tar_test("tar_terra_sds() works", {
       )
     )
   })
-  targets::tar_make()
+  targets::tar_make(reporter = "silent")
   x <- targets::tar_read(raster_elevs)
   expect_s4_class(x, "SpatRasterDataset")
   expect_snapshot(x)
diff --git a/tests/testthat/test-tar-terra-tiles.R b/tests/testthat/test-tar-terra-tiles.R
@@ -44,7 +44,7 @@ targets::tar_test("tar_terra_tiles() works", {
     manifest[manifest$name == "rast_split", ][["command"]],
     "set_window(my_map, terra::ext(rast_split_exts))"
   )
-  targets::tar_make()
+  targets::tar_make(reporter = "silent")
   expect_true(all(is.na(targets::tar_meta()$error)))
 })
 
@@ -83,7 +83,7 @@ targets::tar_test("recombined tiles are equal to original", {
       )
     )
   })
-  targets::tar_make()
+  targets::tar_make(reporter = "silent")
   targets::tar_load(
     c(
       my_map,
diff --git a/tests/testthat/test-tar-terra-vect.R b/tests/testthat/test-tar-terra-vect.R
@@ -19,7 +19,7 @@ targets::tar_test("tar_terra_vect() works", {
       )
     )
   })
-  targets::tar_make()
+  targets::tar_make(reporter = "silent")
   x <- targets::tar_read(test_terra_vect)
   y <- targets::tar_read(test_terra_vect_shz)
   expect_s4_class(x, "SpatVector")
@@ -48,7 +48,7 @@ targets::tar_test("tar_terra_vect() works with dynamic branching", {
       )
     )
   })
-  targets::tar_make()
+  targets::tar_make(reporter = "silent")
   expect_length(targets::tar_read(my_vect_subs), 2)
 })
 
@@ -68,7 +68,7 @@ targets::tar_test("tar_terra_vect() works with multiple workers (tests un/marsha
       )
     )
   })
-  targets::tar_make()
+  targets::tar_make(reporter = "silent")
   expect_true(all(is.na(targets::tar_meta()$error)))
   expect_s4_class(targets::tar_read(vect1), "SpatVector")
 })
diff --git a/tests/testthat/test-tar-terra-vrt.R b/tests/testthat/test-tar-terra-vrt.R
@@ -17,7 +17,7 @@ targets::tar_test("tar_terra_vrt() works", {
     )
   })
 
-  targets::tar_make()
+  targets::tar_make(reporter = "silent")
 
   expect_true(all(is.na(targets::tar_meta()$error)))
 

Original file line number	Diff line number	Diff line change
`@@ -113,6 +113,7 @@ tar_terra_rast <- function(`
`113`	`113`
`114`	`114`	`if (preserve_metadata == "gdalraster_sozip") {`
`115`	`115`	`check_pkg_installed("gdalraster")`
	`116`	`+ check_gdal_sozip()`
`116`	`117`	`}`
`117`	`118`
`118`	`119`	# ensure that user-passed `resources` doesn't include `custom_format`
Original file line number	Diff line number	Diff line change
`@@ -11,7 +11,7 @@ targets::tar_test("geotargets_options_get() retrieves options in correct priorit`
`11`	`11`	`)`
`12`	`12`	`)`
`13`	`13`	`})`
`14`		`- targets::tar_make()`
	`14`	`+ targets::tar_make(reporter = "silent")`
`15`	`15`	`expect_identical(`
`16`	`16`	`geotargets::geotargets_option_get("gdal.raster.driver"),`
`17`	`17`	`"GIF"`