extendr / rextendr

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

Do not use `pretty_rel_path()` outside of the package directory #83

Closed yutannihilation closed 3 years ago

yutannihilation commented 3 years ago

I found it's not very pretty when it's outside of the current package directory.

build directory: /tmp/RtmpOxoyTp/file758976b349de

    Updating crates.io index
   Compiling rextendr3 v0.0.1 (/tmp/RtmpOxoyTp/file758976b349de)
    Finished dev [unoptimized + debuginfo] target(s) in 2.51s
✓ Writting wrappers to '../../../../tmp/RtmpOxoyTp/file758976b349de/target/extendr_wrappers.R'.
clauswilke commented 3 years ago

Yeah, it's not pretty and not useful. The absolute path would be better here. In general, I prefer absolute paths as it helps me find things when debugging, but I'm Ok with the relative path within a package.

Ilia-Kosenkov commented 3 years ago

There is a fix to this (I think) in #76: it outputs absolute path if package root cannot be found. What you observe is the current behavior in main: fs::path_rel creates a relative path to ".", hence this mess.

Ilia-Kosenkov commented 3 years ago

Using #76:

Windows ``` r rextendr::rust_function("fn test_fn() -> &'static str { \"Hello world!\"}") #> build directory: C:\Users\[redacted]\AppData\Local\Temp\RtmpquP6lm\file51ac6f62432a #> v Writting wrappers to C:/Users/[redacted]/AppData/Local/Temp/RtmpquP6lm/file51ac6f62432a/target/extendr_wrappers.R. test_fn() #> [1] "Hello world!" ``` Created on 2021-03-15 by the [reprex package](https://reprex.tidyverse.org) (v1.0.0)
Ubuntu @ WSL2 ``` r rextendr::rust_function("fn test_fn() -> &'static str { \"Hello world!\"}") #> build directory: /tmp/RtmpOj94s3/file5134b8a71b4 #> ✔ Writting wrappers to /tmp/RtmpOj94s3/file5134b8a71b4/target/extendr_wrappers.R. test_fn() #> [1] "Hello world!" ``` Created on 2021-03-15 by the [reprex package](https://reprex.tidyverse.org) (v1.0.0)
clauswilke commented 3 years ago

@Ilia-Kosenkov If you consider this issue addressed with #76, please go ahead and close.

Ilia-Kosenkov commented 3 years ago

@clauswilke, I believe so. Let us close it for now.

yutannihilation commented 3 years ago

Thanks for fixing!