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

`msys2` is no longer needed on Windows? #91

Closed Ilia-Kosenkov closed 3 years ago

Ilia-Kosenkov commented 3 years ago

It seems we do not need msys2 dependency on Windows. My local Windows machines have no msys2 or rtools on their PATHs, I only use RTOOLS40_HOME environment variable, yet I am able to run all tests.

When performing dynamic compilation, we inject path to rtools here https://github.com/extendr/rextendr/blob/09c830538e0f811ec49b4abaf4507870b476aa53/R/source.R#L225-L244 And when Rust code is part of the package and is compiled with Makevars, rtools is added to the PATH by R itself. In both cases, we get the correct dependencies.

I removed these dependencies from CI here https://github.com/extendr/rextendr/blob/09c830538e0f811ec49b4abaf4507870b476aa53/.github/workflows/R-CMD-check.yaml#L72-L73 https://github.com/extendr/rextendr/blob/09c830538e0f811ec49b4abaf4507870b476aa53/.github/workflows/test_pkg_gen.yaml#L70-L71 and these are outputs from two primary CIs:

clauswilke commented 3 years ago

Is this because Rtools was updated incorporating the gcc static compile fix or is there some other reason? Either way, it's good to know that the build environment is getting simpler.

Ilia-Kosenkov commented 3 years ago

Honestly, I am not sure. Rtools has not been updated (yet). I hope someone else (with a Windows machine) verifies this. This can be caused by any number of reasons, including changes in rust toolchains, changes in extendr dependencies, changes in R (4.0.3 -> 4.0.4). The important thing is that it works for stable-msvc, cross-compiling. I think with stable-gnu there still will be problems until new Rtools is released.

Anyway, if my observation is correct, this means there is nothing else needed for extendr usage other than correctly configured rust.

yutannihilation commented 3 years ago

I don't remember the discussions well, but is msys2 needed only for the cases of using bindgen?

CGMossa commented 3 years ago

Msys is used for clang, which is needed for the bindgen use, in the build script for the sys package librsys.

On Mon, Mar 22, 2021, 00:48 Hiroaki Yutani @.***> wrote:

I don't remember the discussions well, but is msys2 needed only for the cases of using bindgen?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/extendr/rextendr/issues/91#issuecomment-803682850, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIDVSFXETG4NUI6DEITPOTTE2AUJANCNFSM4ZPBDOXQ .

yutannihilation commented 3 years ago

Thanks, I meant, the CI passes without msys2 maybe because we don't use bindgen on this repo? (Not sure whether rextendr should provide an option to enable it or not, though)

Ilia-Kosenkov commented 3 years ago

We do not use bindgen here because there is no reason to -- pre-computed bindings save a lot of time & cpu cycles for end users. However, there was some issue before, which was blocking us from using rust without msys2, see discussions in #19. The problem with gnu toolchain still exists (I think), but msvc works because we inject Rtools path (this is the best guess I have). I checked CI PATH, and it does not contain any references to msys2 by default, so I believe it indeed works without msys2 (unless there is some weird caching I am unaware of).

Another thing is that since #19 extendr axed some dependencies, and we also reference only extendr-api of the latest published version, while prior to that we additionally had either extendr-macros or extendr-engine, which likely pulled in more dependencies.

yutannihilation commented 3 years ago

Ah, got it. I forgot we used gnu toolchain at that time... Anyway, I'll confirm this on my Windows laptop (probably within this week).

yutannihilation commented 3 years ago

@Ilia-Kosenkov While the test on package generation works, the other test of R CMD check fails on my local without having msys2 on PATH. For the success of CI, my guess is that GitHub Actions CI includes msys2 by default whether we explicitly add it or not. But I have no idea about the difference between my setup and yours...

==> devtools::test()

Loading rextendr
Testing rextendr
�� |  OK F W S | Context
/ |   0       | eval                                                                                                         Updating crates.io index
   Compiling winapi-build v0.1.1
   Compiling winapi v0.3.9
   Compiling winapi v0.2.8
   Compiling proc-macro2 v1.0.24
   Compiling unicode-xid v0.2.1
   Compiling syn v1.0.65
   Compiling extendr-engine v0.2.0
   Compiling lazy_static v1.4.0
   Compiling kernel32-sys v0.2.2
   Compiling quote v1.0.9
   Compiling extendr-macros v0.2.0
   Compiling libR-sys v0.2.1
   Compiling extendr-api v0.2.0
   Compiling rextendr1 v0.0.1 (C:\Users\HIROAK~1\AppData\Local\Temp\RtmpKkE6Fn\file2248170e408d)
error: linking with `x86_64-w64-mingw32-gcc` failed: exit code: 1
  |
  = note: "x86_64-w64-mingw32-gcc" "-fno-use-linker-plugin" "-Wl,--nxcompat" "-Wl,--dynamicbase" "-Wl,--disable-auto-image-base" "-m64" "-Wl,--high-entropy-va" "C:\\Users\\hiroaki-yutani\\.rustup\\toolchains\\stable-x86_64-pc-windows-msvc\\lib\\rustlib\\x86_64-pc-windows-gnu\\lib\\rsbegin.o" "-L" ...snip...  "-Wl,-Bdynamic" "-lR" "-ladvapi32" "-lws2_32" "-luserenv" "-lgcc_eh" "-l:libpthread.a" "-lmsvcrt" "-lmingwex" "-lmingw32" "-lgcc" "-lmsvcrt" "-luser32" "-lkernel32" "C:\\Users\\hiroaki-yutani\\.rustup\\toolchains\\stable-x86_64-pc-windows-msvc\\lib\\rustlib\\x86_64-pc-windows-gnu\\lib\\rsend.o"
  = note: C:/rtools40/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/8.3.0/../../../../x86_64-w64-mingw32/bin/ld.exe: cannot find -lgcc_eh
          collect2.exe: error: ld returned 1 exit status

error: aborting due to previous error

error: could not compile `rextendr1`
yutannihilation commented 3 years ago

While the test on package generation works

I also have no idea why this succeeds...

Ilia-Kosenkov commented 3 years ago

Ah I see, it is the libgcc_eh problem that will be solved as soon as rtools ships our patch.

I will verify this with a clean rtools installation, though I have no explanation why it works on CI - I printed PATH variable and manually checked there is no PATH to msys2 included.

yutannihilation commented 3 years ago

I printed PATH variable and manually checked there is no PATH to msys2 included.

Thanks, you are correct here, but it seems some of the paths seem to matter; if I set Sys.setenv(PATH = "C:/R/bin;C:/rtools40/usr/bin;C:/Rust/.cargo/bin") explicitly, it fails.

https://github.com/yutannihilation/rextendr/runs/2208973449?check_suite_focus=true#step:11:332

Ilia-Kosenkov commented 3 years ago

Interesting, perhaps one of the PATH values point to some other library/app that uses libgcc, and that is how it gets resolved. The error here suggests that rtools fix will allow us to get rid of msys2.

yutannihilation commented 3 years ago

Yeah, let's close this issue when it's shipped.

clauswilke commented 3 years ago

Can we close this issue now?

Ilia-Kosenkov commented 3 years ago

Can we close this issue now?

Not 100% sure, we still include msys here: https://github.com/extendr/rextendr/blob/00ce4650ee044e4b7c66ebeaecaa65c6d5ec7024/.github/workflows/R-CMD-check.yaml#L72-L73 I am going to look into this to make sure that we need only Rtools40v2 to make it work.