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

pretty_rel_path() fails on Windows with R-devel (R 4.3) #238

Closed yutannihilation closed 1 year ago

yutannihilation commented 1 year ago

This is reproducible on my local.

https://github.com/extendr/extendr/actions/runs/4210717913/jobs/7308589655#step:18:123

   ══ Failed tests ════════════════════════════════════════════════════════════════
  ── Failure ('test-helper-functions.R:32'): `pretty_rel_path()` works ───────────
  pretty_rel_path(...) (`actual`) not equal to normalizePath(...) (`expected`).

  actual vs expected
  - "R/extendr-wrappers.R"
  + "C:/Users/runneradmin/AppData/Local/Temp/RtmpQ9SQyF/working_dir/RtmpcB6a2L/file6f469214d6a/testpkg/R/extendr-wrappers.R"

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

I am looking into this as well right now. I wonder what has changed.

yutannihilation commented 1 year ago

It seems the difference is that this line doesn't fail on R-devel

https://github.com/extendr/rextendr/blob/d22584825802c928d8dc1294ea9ec85e2e353a64/R/track_rust_source.R#L39

in case of this

https://github.com/extendr/rextendr/blob/d22584825802c928d8dc1294ea9ec85e2e353a64/tests/testthat/test-helper-functions.R#L33-L36

Ilia-Kosenkov commented 1 year ago

Strange, I cannot reproduce it locally with devtools::test(), nor with rcmdcheck::rcmdcheck()

Ilia-Kosenkov commented 1 year ago

Ah wait, found one https://github.com/extendr/rextendr/blob/d22584825802c928d8dc1294ea9ec85e2e353a64/tests/testthat/test-helper-functions.R#L22-L28

>  expect_equal(
+     pretty_rel_path(
+       file.path(pkg_root, "R", "extendr-wrappers.R"),
+       file.path(pkg_root, "src", "rust", "src", "lib.rs")
+     ),
+     "R/extendr-wrappers.R"
+   )
Error: pretty_rel_path(...) (`actual`) not equal to "R/extendr-wrappers.R" (`expected`).

`actual`:   "E:/.../rextendr/R/extendr-wrappers.R"
`expected`: "R/extendr-wrappers.R"
>
yutannihilation commented 1 year ago

Hmm, it's different one, but probably the same cause?

Ilia-Kosenkov commented 1 year ago

I am baffled. So this fragment https://github.com/extendr/rextendr/blob/d22584825802c928d8dc1294ea9ec85e2e353a64/tests/testthat/test-helper-functions.R#L32-L41 tests that if we search for a file starting outside of the package directory, we fail and return the full path. It works as expected everywhere (including locally in R-devel), but on CI it for some reason... succeeds and returns relative to the project root path.

yutannihilation commented 1 year ago

including locally

No, it fails on my local.

It seems R-devel recently made several changes to remove the limitation on the path length on Windows, so it might be that it no longer uses 8.3 short name in some cases and it mattered. Just a wild guess.

Ilia-Kosenkov commented 1 year ago

I am not sure, since we use normalizePath() which should expand that. It seems to be Windows-specific. If you cannot pinpoint the issue (and it is not an issue per se, since we expect pretty_rel_path() to fail yet it succeeds), and since I am unable to reproduce it, should we skip this test for now and revisit it later?

yutannihilation commented 1 year ago

The minimal reprex.

R 4.2.2

f <- function() {
  pkg_root <- withr::local_tempdir()
  usethis::create_package(pkg_root)
  setwd(pkg_root)

  path <- file.path(pkg_root, "..")
  rprojroot::find_package_root_file(path = path)
}

f()
#> ✔ Setting active project to 'C:/Users/Yutani/AppData/Local/Temp/RtmpEljiyX/file330c1c726ce4'
#> ✔ Creating 'R/'
#> ✔ Writing 'DESCRIPTION'
#> Package: file330c1c726ce4
#> Title: What the Package Does (One Line, Title Case)
#> Version: 0.0.0.9000
#> Authors@R (parsed):
#>     * First Last <first.last@example.com> [aut, cre] (YOUR-ORCID-ID)
#> Description: What the package does (one paragraph).
#> License: `use_mit_license()`, `use_gpl3_license()` or friends to pick a
#>     license
#> Encoding: UTF-8
#> Roxygen: list(markdown = TRUE)
#> RoxygenNote: 7.2.3
#> ✔ Writing 'NAMESPACE'
#> ✔ Setting active project to '<no active project>'
#> Error: No root directory found in C:/Users/Yutani/AppData/Local/Temp/RtmpEljiyX or its parent directories. Root criterion: contains a file "DESCRIPTION" with contents matching "^Package: "

Created on 2023-02-21 with reprex v2.0.2

R 4.3.0

f <- function() {
  pkg_root <- withr::local_tempdir()
  usethis::create_package(pkg_root)
  setwd(pkg_root)

  path <- file.path(pkg_root, "..")
  rprojroot::find_package_root_file(path = path)
}

f()
#> ✔ Setting active project to 'C:/Users/Yutani/AppData/Local/Temp/Rtmp2DYIJ0/file54246b1212a7'
#> ✔ Creating 'R/'
#> ✔ Writing 'DESCRIPTION'
#> Package: file54246b1212a7
#> Title: What the Package Does (One Line, Title Case)
#> Version: 0.0.0.9000
#> Authors@R (parsed):
#>     * First Last <first.last@example.com> [aut, cre] (YOUR-ORCID-ID)
#> Description: What the package does (one paragraph).
#> License: `use_mit_license()`, `use_gpl3_license()` or friends to pick a
#>     license
#> Encoding: UTF-8
#> Roxygen: list(markdown = TRUE)
#> RoxygenNote: 7.2.3
#> ✔ Writing 'NAMESPACE'
#> ✔ Setting active project to '<no active project>'
#> [1] "."

Created on 2023-02-21 with reprex v2.0.2

yutannihilation commented 1 year ago

should we skip this test for now and revisit it later?

I'm fine with either, but I hope we can help upstream (R itself or rprojroot) to figure out what the problem is.

Ilia-Kosenkov commented 1 year ago

Soo, now I can reproduce it locally. The issue was that I had a very old R-devel version, still 4.3 but from last year. I pulled the most recent one, and now observe the same behaviour as on CI. Will try to debug locally.

Ilia-Kosenkov commented 1 year ago

Here is a reprex for you, this reliably fails on release but works (evaluating path to ".") on devel.


pkg_path <- tempfile()
tryCatch(
    {
        pkg <- usethis::create_package(pkg_path)
        setwd(pkg)
        path_outside <- file.path(pkg_path, "..")
        projroot <- rprojroot::find_package_root_file(path = path_outside)
        print(projroot)
    },
    finally = unlink(pkg_path)
)
sessionInfo()
yutannihilation commented 1 year ago

Fixed by https://github.com/wch/r-source/commit/d444c07b7fe765ce65224faa2e85f0819a189e1a