extendr / rextendr

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

Crash when calling `rextendr:::document()` in R 4.2 #191

Closed arcresu closed 2 years ago

arcresu commented 2 years ago

I installed rextendr with renv::install("extendr/rextendr") (currently at 1df2600ef386c83e47261a36c31076632035e606). When I call rextendr::document() I get:

> rextendr::document()
✔ Saving changes in the open files.
ℹ Generating extendr wrapper functions for package: mypackage.
Error in if (!nzchar(path)) { : the condition has length > 1
In addition: Warning message:
In !nzchar(package_root) || !stringi::stri_detect_fixed(str = path,  :
  'length(x) = 17 > 1' in coercion to 'logical(1)'

I tracked down the call chain that triggers this:

It seems like pretty_rel_path is assuming it'd only ever be called with a scalar path, so either that assumption is wrong and it's buggy, or else needs_compilation should be fixed to avoid passing it a vector of paths.

Reprex:

# mkdir tmp && cd tmp
usethis::create_package(".")
rextendr::use_extendr()
rextendr::document()
system("touch src/rust/src/a.rs")
system("touch src/rust/src/b.rs")
rextendr::document()
CGMossa commented 2 years ago

I tried this on main and got the same issue. Did we rename use_rextendr()?

tempdir() -> temp_package_location
temp_package_location
usethis::create_package(temp_package_location)
rextendr::use_extendr()
rextendr::document()
system("touch src/rust/src/a.rs")
system("touch src/rust/src/b.rs")
rextendr::document()

Results in:

> rextendr::document()
✔ Saving changes in the open files.
ℹ Generating extendr wrapper functions for package: RtmpMNe9BF.
Error in if (!nzchar(path)) { : the condition has length > 1
In addition: Warning message:
In !nzchar(package_root) || !stringi::stri_detect_fixed(str = path,  :
  'length(x) = 2 > 1' in coercion to 'logical(1)'

And a traceback yields:

6: pretty_rel_path(modified_files_paths, search_from = path)
5: map(.x, .f, ...)
4: purrr::walk(pretty_rel_path(modified_files_paths, search_from = path), 
       ~ui_i("File {.file {.x}} has been modified since last compilation."))
3: needs_compilation(path, quiet)
2: register_extendr(path = pkg, quiet = quiet)
1: rextendr::document()
CGMossa commented 2 years ago

This snippet


  # If `package_root` is empty or not a parent of `path`,
  # return `path` unchanged (for simplicity).
  if (
    !nzchar(package_root) ||
      !stringi::stri_detect_fixed(
        str = path,
        pattern = package_root,
        case_insensitive = TRUE
      )
  ) {
    return(path)
  }

Is problematic because:

Browse[2]> stringi::stri_detect_fixed(
+         str = path,
+         pattern = package_root,
+         case_insensitive = TRUE
+       )
[1] TRUE TRUE
Browse[2]> path
[1] "C:/Users/angus/AppData/Local/Temp/RtmpQziKDf/dir14a8302f61f8/src/rust/src/a.rs"
[2] "C:/Users/angus/AppData/Local/Temp/RtmpQziKDf/dir14a8302f61f8/src/rust/src/b.rs"
yutannihilation commented 2 years ago

Did we rename use_rextendr()?

No. It's use_extendr() (without "r").

arcresu commented 2 years ago

Sorry, that was just a typo; I was using use_extendr(). The important part was just that there has to be more than one modified rust source file for the problem to manifest.

I tried out the fix in 216e90c1f6551be6ba998c07704f85d214f9062e and confirm it works for me. Thanks!