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

Rely on updated pkgbuild::needs_compilation() #221

Closed yutannihilation closed 1 year ago

yutannihilation commented 1 year ago

Fix #215

(Sorry, I mistakenly committed this directly to the main branch... I reverted)

Ilia-Kosenkov commented 1 year ago

Should track_rust_source file be removed completely? Do we use its functions anywhere else?

Also, I believe it is worth updating the changelog.

yutannihilation commented 1 year ago

Oh, it might be. I'll try it.

I'm still figuring out, but I think this doesn't introduce any user-visible changes. What do you think?

Ilia-Kosenkov commented 1 year ago

Hm, you are correct. We used pkgbuild before. The only visible change is the minimum required version :)

yutannihilation commented 1 year ago

Yeah. One notable progress is that the developers now can just press the "Load all" menu of RStudio as long as no signatures are changed. But this is not our archivement...

yutannihilation commented 1 year ago

Should track_rust_source file be removed completely? Do we use its functions anywhere else?

I checked. In short, there's not many functions that can be removed.

pretty_rel_path() is used here; pretty_rel_single_path() and on_error_return_default() are used inside the function.

https://github.com/extendr/rextendr/blob/ae38dfc0ffa3fef294fc97424c7c19580968a787/R/register_extendr.R#L89

get_library_path() is used here.

https://github.com/extendr/rextendr/blob/ae38dfc0ffa3fef294fc97424c7c19580968a787/R/register_extendr.R#L68

find_newer_files_than() is used here get_file_info() is used inside the function.

https://github.com/extendr/rextendr/blob/ae38dfc0ffa3fef294fc97424c7c19580968a787/R/register_extendr.R#L88

Ilia-Kosenkov commented 1 year ago

Sorry I missed your comment here. It feels like get_file_info() should no longer be used?

yutannihilation commented 1 year ago

No, I meant get_file_info() is used in find_newer_files_than(), which is used in register_extendr().

https://github.com/extendr/rextendr/blob/c64b0dbd1152c395c82e9191d915c794accc9c27/R/track_rust_source.R#L230-L235

Ilia-Kosenkov commented 1 year ago

Ok, now I see. Perhaps it can later be simplified since we only want to check if wrappers are newer than dll or not.

yutannihilation commented 1 year ago

Thanks for the hint! I replaced it with bare file.info().

Ilia-Kosenkov commented 1 year ago

Do you feel like it is ready for review? Or are there other things you need to address first?

yutannihilation commented 1 year ago

Yes, I think so. I was wondering if I can reduce more code, but it seems this is all for now.

Ilia-Kosenkov commented 1 year ago

Hm, it blew up because of {purrr}. Strange

DLL 'purrr' not found: maybe not installed for this architecture?

yutannihilation commented 1 year ago

Yeah, I have no idea. I hope time will solve...