extendr / rextendr

An R package that helps scaffolding extendr-enabled packages or compiling Rust code dynamically
https://extendr.github.io/rextendr/
Other
181 stars 27 forks source link

feat: Add a field to DESCRIPTION to notify which version of `rextendr` the package was created with #225

Closed eitsupi closed 1 year ago

eitsupi commented 1 year ago

Close #222

Like roxygen2 (r-lib/roxygen2#338), record the version of the rextendr used to create the package in DESCRIPTION to prevent the use of older versions.

It seems that the field name for this needs to start with Config/ ? https://github.com/wch/r-source/blob/d1618921e670f1607da77ba850949f8312abdff6/src/library/tools/R/QC.R#L7204-L7213

eitsupi commented 1 year ago

Sorry, but I can't seem to fix the test because I can't reproduce the problem locally (it fails to compile in the first place).

Fixed it after I rebuilt the container. I will look at it some more...

> devtools::test(filter = "use_extendr")
ℹ Testing rextendr
Starting 1 test process
✔ | F W S  OK | Context
✖ | 3      22 | use_extendr [53.2s]                                                                                           
──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
Error (test-use_extendr.R:94): use_extendr() handles R packages with dots in the name
<rextendr_error/rlang_error/error/condition>
Error in `value[[3L]](cond)`: Failed to generate wrapper functions.
x in callr subprocess.
Backtrace:
 1. rextendr::document()
      at test-use_extendr.R:94:2
 2. rextendr::register_extendr(path = pkg, quiet = quiet)
      at rextendr/R/rextendr_document.R:17:2
 3. base::tryCatch(...)
      at rextendr/R/register_extendr.R:88:2
 4. base (local) tryCatchList(expr, classes, parentenv, handlers)
 5. base (local) tryCatchOne(expr, names, parentenv, handlers[[1L]])
 6. value[[3L]](cond)

Error (test-use_extendr.R:103): use_extendr() handles R package name, crate name and library name separately
<rextendr_error/rlang_error/error/condition>
Error in `value[[3L]](cond)`: Failed to generate wrapper functions.
x in callr subprocess.
Backtrace:
 1. rextendr::document()
      at test-use_extendr.R:103:2
 2. rextendr::register_extendr(path = pkg, quiet = quiet)
      at rextendr/R/rextendr_document.R:17:2
 3. base::tryCatch(...)
      at rextendr/R/register_extendr.R:88:2
 4. base (local) tryCatchList(expr, classes, parentenv, handlers)
 5. base (local) tryCatchOne(expr, names, parentenv, handlers[[1L]])
 6. value[[3L]](cond)

Error (test-use_extendr.R:131): Warn if using older rextendr
<rextendr_error/rlang_error/error/condition>
Error in `value[[3L]](cond)`: Failed to generate wrapper functions.
x in callr subprocess.
Backtrace:
  1. testthat::expect_message(document(), "Installed rextendr is older than the version used with this package")
       at test-use_extendr.R:131:2
  7. rextendr::document()
  8. rextendr::register_extendr(path = pkg, quiet = quiet)
       at rextendr/R/rextendr_document.R:17:2
  9. base::tryCatch(...)
       at rextendr/R/register_extendr.R:88:2
 10. base (local) tryCatchList(expr, classes, parentenv, handlers)
 11. base (local) tryCatchOne(expr, names, parentenv, handlers[[1L]])
 12. value[[3L]](cond)
──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────

══ Results ═══════════════════════════════════════════════════════════════════════════════════════════════════════════════════
Duration: 56.8 s

[ FAIL 3 | WARN 0 | SKIP 0 | PASS 22 ]
Ilia-Kosenkov commented 1 year ago

Ok, I tested it locally, seems to work fine. However, I am not entirely sure about the path in the config name. Is this because we want to avoid R CMD check checking this field? Why Roxygen can have their own name while we cannot?

yutannihilation commented 1 year ago

Is this because we want to avoid R CMD check checking this field?

In my understanding, we don't really need this, but it's a common practice among tidyverse packages to use Config/ nowadays. In theory, we can put any field as the WRE says:

There is no restriction on the use of other fields not mentioned here (but using other capitalizations of these field names would cause confusion). Fields Note, Contact (for contacting the authors/developers9) and MailingList are in common use. Some repositories (including CRAN and R-forge) add their own fields.

It seems common field names like Roxygen are manually excluded: https://github.com/wch/r-source/blob/65710ee970c905903c497e806194b41b04385562/src/library/tools/R/utils.R#L1364-L1375. I guess the reason why roxygen2 has its own field is that it was born long before Config/ is popular.

Ilia-Kosenkov commented 1 year ago

Ok, sounds fair. @eitsupi, let me know if you are finished with this PR -- I need to force-merge it to bypass 32-bit failing CI.

eitsupi commented 1 year ago

Thank you all for your help.

@eitsupi, let me know if you are finished with this PR -- I need to force-merge it to bypass 32-bit failing CI.

The work is done and I would appreciate it if you could merge it if the field name is ok. (The field name is obviously difficult to change later once release!)

Ilia-Kosenkov commented 1 year ago

Thank you for contributing @eitsupi !