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

Use rtools42 #187

Closed yutannihilation closed 2 years ago

yutannihilation commented 2 years ago

This pull request is the successor of #171.

Note that this pull request assumes we use stable-gnu on R-devel because it's difficult to support both, but we don't decide which to use stable-gnu or stable-msvc. If we choose the latter, I'll rewrite the corresponding codes.

yutannihilation commented 2 years ago

No idea why only the test of dot name fail in this way. I cannot reproduce this issue on my local. This needs some investigation...

E> x86_64-w64-mingw32.static.posix-gcc.exe: error: C:\Users\runneradmin\AppData\Local\Temp\RtmpGIYi8A\working_dir\Rtmpa4uQZH\file6e0254a6d7\package.with.dots\src\./rust/target\release\build\winapi-x86_64-pc-windows-gnu-4bcab0af3bd55335\build_script_build-4bcab0af3bd55335.build_script_build.f5023671-cgu.9.rcgu.o: No such file or directory E> E> E> error: could not compile `winapi-x86_64-pc-windows-gnu` due to previous error

Ilia-Kosenkov commented 2 years ago

I will try to look into these errors later today. Need to first configure R-devel & ucrt toolchain locally. Indeed it is strange, as a hundred of other tests succeed.

mati865 commented 2 years ago

No idea why only the test of dot name fail in this way. I cannot reproduce this issue on my local. This needs some investigation...

E> x86_64-w64-mingw32.static.posix-gcc.exe: error: C:\Users\runneradmin\AppData\Local\Temp\RtmpGIYi8A\working_dir\Rtmpa4uQZH\file6e0254a6d7\package.with.dots\src\./rust/target\release\build\winapi-x86_64-pc-windows-gnu-4bcab0af3bd55335\build_script_build-4bcab0af3bd55335.build_script_build.f5023671-cgu.9.rcgu.o: No such file or directory E> E> E> error: could not compile `winapi-x86_64-pc-windows-gnu` due to previous error

I believe this hits Windows path length limit since GCC/Binutils do not add manifest allowing to use longer than default paths.

Ilia-Kosenkov commented 2 years ago

I believe this hits Windows path length limit since GCC/Binutils do not add manifest allowing to use longer than default paths.

This is an interesting idea. The path you quote seems to be 262 characters long, while Windows (by default) supports only 260 characters. It is possible that developers like myself or @yutannihilation have workstations configured to use longer paths and therefore not observe this error. We can test it by changing that name to, say, pkg.w.dts.

It is also a sign for us to investigate support of CARGO_TARGET_DIR (or whatever variable controls the location of cargo artifacts). Also, \src\./rust/target/ suggests that perhaps the extra ./ is unnecessary.

yutannihilation commented 2 years ago

Oh, thanks. It can explain the problem. Let's try some shorter package name.

yutannihilation commented 2 years ago

🎉

Ilia-Kosenkov commented 2 years ago

We definitely need to re-consider temporary cargo build directories to avoid similar problems in the future. Great job by the way!

yutannihilation commented 2 years ago

Thanks! I'm yet to understand what the problem is, but I guess using a temp dir is a trade-off between the compilation speed. Anyway let's discuss in another issue.

Thanks @mati865 for your great insight. It really helped us!

yutannihilation commented 2 years ago

What I tried so far:

yutannihilation commented 2 years ago

This pull request is ready for review now, but probably the description is too terse now. I'll write more, but my priority is https://github.com/extendr/libR-sys/pull/94 for now. But feel free to ask questions...

yutannihilation commented 2 years ago

@Ilia-Kosenkov Can we merge this? Sorry I didn't complete my promise of writing more detailed description..., but I think this needs some load test.

yutannihilation commented 2 years ago

Thanks! If you find any problem when using rextendr with R 4.2, please let me know.