search

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

On Windows, default rust needs to be gnu, not msvc #19

Closed clauswilke closed 3 years ago

clauswilke commented 3 years ago

Apparently, under some circumstances, the default Rust on Windows needs to be gnu, not msvc.

See: https://github.com/extendr/rextendr/issues/15#issuecomment-757944529 and https://github.com/extendr/rextendr/pull/7#issuecomment-758272287

Is there any way to remove this requirement?

cc @Ilia-Kosenkov @CGMossa @yutannihilation

clauswilke commented 3 years ago

This is particularly frustrating since building an R package with extendr on Windows requires msvc to be the default toolchain. I would prefer to be able to tell people to always make msvc the default.

CGMossa commented 3 years ago

I can offer this workaround, that I think is fairly okay to use..

rustup run stable-gnu cargo build

Even if the default is nightly-msvc, it would run this under that toolchain and host.

Although, I'm not sure rustup is on linux/mac.

Loosely, the build commands would then be like

#...
  status <- system2(
    command = "rustup",
    args = c(
      "run",
      "stable-gnu",
      "cargo",
      "build",
...
yutannihilation commented 3 years ago

@CGMossa Could you share the error message you got when you run it with msvc default? I'm curious about the cause, though I'm not sure if it's what we can fix or not.

CGMossa commented 3 years ago

I did share it, see the link that Claus posted, or here:

error[E0463]: can't find crate for `std`                       
  |
  = note: the `x86_64-pc-windows-gnu` target may not be installed

While I'm at it: Using cargo +stable-gnu build does not work, as I think you can only write few things in cargo +<here>.

yutannihilation commented 3 years ago

Oh, I see. Sorry. I thought you got some different error after you added the target. Hmm...

clauswilke commented 3 years ago

Before implementing some workaround I'd like to understand what the issue is. We're just calling cargo build on a regular Rust crate, I don't understand why this would be a problem. We're doing this exact same thing when building R packages or extendr standalone binaries and we don't have errors there.

CGMossa commented 3 years ago

Ok.

I added rustup target add x86_64-pc-windows-gnu and somehow that added it to stable-msvc. This made a new error appear: 

error: linker `x86_64-w64-mingw32-gcc` not found
  |
  = note: The system cannot find the file specified. (os error 2)

Then I added gcc that comes from rtools onto the path; Via

PS C:\Users\angus> $env:Path += ";C:\rtools40\usr\bin\;C:\rtools40\mingw64\bin\";

And then I get another error:

[omitted]
  = 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

And now I think this is just not possible to get past.

> Sys.which("ld")
                                  ld 
"C:\\rtools40\\mingw64\\bin\\ld.exe"

That's it from me; I don't know what else to do about this.

yutannihilation commented 3 years ago

Thanks. So, maybe we came back to this issue...?

https://github.com/rust-lang/cargo/issues/8990#issuecomment-748511774

(Sorry, I don't follow the discussion around this topic during the last year-end... It needs some time for me to catch up.)

clauswilke commented 3 years ago

cannot find -lgcc_eh is normally the problem when you're not making msvc the default toolchain. Weird. Maybe some problem with paths that get set when R is running. It should not be using the rtools linker but instead the msvc one, I believe.

clauswilke commented 3 years ago

In the extendr github action scripts, we're explicitly adding C:\msys64\mingw64\bin to the path: https://github.com/extendr/extendr/blob/f714309fc6806f953cf82a6a30b0863a5d4eff1d/.github/workflows/test.yml#L231

We may need something similar here. You can set environment variables in .Renviron: https://rstats.wtf/r-startup.html

clauswilke commented 3 years ago

Ok, I think I'm starting to see how this all fits together. The problem is always the linker. For standalone Rust binaries, we need to use the msvc tool chain to get things linking correctly. For dynamic libraries (the use case here), we would also need the msvc tool chain. But when we're building an R package with extendr (as here: https://github.com/extendr/helloextendr/blob/main/src/Makevars.win) we're building a static library that we then let R link. This works fine with the msvc tool chain also, but the actual linking happens with the Rtools tool chain controlled by R.

What is breaking here is trying to build a dynamic library directly using a mixed tool chain. It makes sense that this wouldn't work. We need to make sure that the Rtools ld.exe doesn't get picked when linking the final dynamic library.

Alternatively, we could mimic the approach of building an R package, that is, use cargo only to build a static library and then use R to build a dynamic library. That's probably possible but a little more work on our end. (And I've never done this, so I don't know off the top of my head how to do it. But there's this: https://www.rdocumentation.org/packages/utils/versions/3.6.2/topics/SHLIB)

Ilia-Kosenkov commented 3 years ago

I am sorry I fail to understand what actually triggered the linker error? Was it some simple test code? I have seen a lot of different error combinations on Windows, but what I learned is that everything works if we take msvc toolchain and provide the correct target(s).

Could you please try setting up default toolchain

rustup default stable-x86_64-pc-windows-msvc

and then adding two targets

rustup target add x86_64-pc-windows-gnu --toolchain stable-x86_64-pc-windows-msvc
rustup target add i686-pc-windows-gnu --toolchain stable-x86_64-pc-windows-msvc

If PATH is contaminated by Rtools binaries, all sorts of weird things happen. Remove all references to Rtools subfolder, but ensure that you have a separate env variable RTOOLS40_HOME, which is set by default by the installer.

Then it should work. And if it does not, please provide a minimal reproducible example and detailed output.

yutannihilation commented 3 years ago

Hmm..., I think I followed your instruction, but it failed.

> rust_function("fn add(a:f64, b:f64) -> f64 { a + b }")
build directory: C:\Users\HIROAK~1\AppData\Local\Temp\RtmpKOZj3U\file198858e28f7

    Updating crates.io index
   Compiling rextendr3 v0.0.1 (C:\Users\HIROAK~1\AppData\Local\Temp\RtmpKOZj3U\file198858e28f7)
error: linker `x86_64-w64-mingw32-gcc` not found
  |
  = note: 謖・ｮ壹＆繧後◆繝輔ぃ繧､繝ｫ縺瑚ｦ九▽縺九ｊ縺ｾ縺帙ｓ縲・(os error 2)

error: aborting due to previous error

error: could not compile `rextendr3`

To learn more, run the command again with --verbose.
Error: Rust code could not be compiled successfully. Aborting.
> p <- Sys.getenv("PATH")
> p <- strsplit(p, ";", fixed = TRUE)[[1]]
> idx <- grepl("rtools", p, ignore.case = TRUE)
> p[idx]
character(0)
> Sys.getenv("RTOOLS40_HOME")
[1] "C:\\rtools40"
> system("rustup toolchain list")
stable-i686-pc-windows-gnu
stable-x86_64-pc-windows-gnu
stable-x86_64-pc-windows-msvc (default)
[1] 0
Ilia-Kosenkov commented 3 years ago

@yutannihilation, I see. The missing linker comes from MSYS2, which is added to my path. If you have it installed, could you temporarily add msys2\mingw64\bin to your PATH to see if it resolves the issue?

yutannihilation commented 3 years ago

Thanks, confirmed it works (In my environment, the path seems msys64\mingw64\bin). Then, do you mean we need to require Windows users to install MSYS2 to use rextendr?

``` > library(rextendr) > p <- Sys.getenv("PATH") > Sys.setenv(PATH = paste0("C:\\msys64\\mingw64\\bin", ";", p)) > rust_function("fn add(a:f64, b:f64) -> f64 { a + b }") build directory: C:\Users\HIROAK~1\AppData\Local\Temp\RtmpoVe16y\file307874ec3870 Updating git repository `https://github.com/extendr/extendr` 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.58 Compiling extendr-engine v0.1.11 (https://github.com/extendr/extendr#f714309f) Compiling lazy_static v1.4.0 Compiling kernel32-sys v0.2.2 Compiling quote v1.0.8 Compiling extendr-macros v0.1.11 (https://github.com/extendr/extendr#f714309f) Compiling libR-sys v0.2.1 Compiling extendr-api v0.1.11 (https://github.com/extendr/extendr#f714309f) Compiling rextendr1 v0.0.1 (C:\Users\HIROAK~1\AppData\Local\Temp\RtmpoVe16y\file307874ec3870) Finished dev [unoptimized + debuginfo] target(s) in 52.52s > add(2.5, 4.7) [1] 7.2 ```
Ilia-Kosenkov commented 3 years ago

@yutannihilation, I reproduced your error by removing msys2 from my path. My observation, we need either:

In both cases we need to provide --target as I did in #16, otherwise x86 R will not be able to load the library (which is rare, but important).

So, my suggestion is to allow for toolchain optional parameter to override what toolchain is used. This can be passed to various rust_* functions. The default NULL value corresponds to the default toolchain (i.e. no extra parameter). I believe this will help people with complex environment setups to control how the library is built. It can be also tested in CI.

clauswilke commented 3 years ago

@Ilia-Kosenkov Feel free to implement the toolchain parameter you suggest.

Also, it would be great if you could add brief instructions on how to get things going on Windows here: https://github.com/extendr/rextendr/blob/main/vignettes/setting_up_rust.Rmd

Ilia-Kosenkov commented 3 years ago

@clauswilke, About to open PR, finishing with the docs.

yutannihilation commented 3 years ago

So, my suggestion is to allow for toolchain optional parameter to override what toolchain is used.

Thanks! I now understand what you meant. Confirmed #20 works on my environment.

### Without `toolchain` ``` > rust_function("fn add(a:f64, b:f64) -> f64 { a + b }") build directory: C:\Users\HIROAK~1\AppData\Local\Temp\RtmpWIVDm3\file24205dfb5677 Updating git repository `https://github.com/extendr/extendr` 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.58 Compiling extendr-engine v0.1.11 (https://github.com/extendr/extendr#f714309f) Compiling lazy_static v1.4.0 Compiling kernel32-sys v0.2.2 Compiling quote v1.0.8 Compiling extendr-macros v0.1.11 (https://github.com/extendr/extendr#f714309f) Compiling libR-sys v0.2.1 Compiling extendr-api v0.1.11 (https://github.com/extendr/extendr#f714309f) Compiling rextendr1 v0.0.1 (C:\Users\HIROAK~1\AppData\Local\Temp\RtmpWIVDm3\file24205dfb5677) error: linker `x86_64-w64-mingw32-gcc` not found | = note: 謖・ｮ壹＆繧後◆繝輔ぃ繧､繝ｫ縺瑚ｦ九▽縺九ｊ縺ｾ縺帙ｓ縲・(os error 2) error: aborting due to previous error error: could not compile `rextendr1` To learn more, run the command again with --verbose. Error: Rust code could not be compiled successfully. Aborting. ``` ### With `toolchain` specified ``` > rust_function("fn add(a:f64, b:f64) -> f64 { a + b }", toolchain = "stable-gnu") build directory: C:\Users\HIROAK~1\AppData\Local\Temp\RtmpWIVDm3\file24205dfb5677 Updating crates.io index Compiling winapi-x86_64-pc-windows-gnu v0.4.0 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.58 Compiling extendr-engine v0.1.11 (https://github.com/extendr/extendr#f714309f) Compiling lazy_static v1.4.0 Compiling kernel32-sys v0.2.2 Compiling quote v1.0.8 Compiling extendr-macros v0.1.11 (https://github.com/extendr/extendr#f714309f) Compiling libR-sys v0.2.1 Compiling extendr-api v0.1.11 (https://github.com/extendr/extendr#f714309f) Compiling rextendr2 v0.0.1 (C:\Users\HIROAK~1\AppData\Local\Temp\RtmpWIVDm3\file24205dfb5677) Finished dev [unoptimized + debuginfo] target(s) in 57.77s ```
Ilia-Kosenkov commented 3 years ago

I am reopnening this issue because I am unable to setup simple Windows CI using stable-gnu toolchain. The build fails with linker complaining about lgcc_eh, similar to what was reported here earlier. I will try to investigate this further.

Ilia-Kosenkov commented 3 years ago

The root cause is that setup-r action incorrectly updates PATH when installing rtools. By default, PATH should only contain C:\rtools40\usr\bin to get access to make. setup-r adds C:\rtools40\mingw64\bin, similar to what @CGMossa did in https://github.com/extendr/rextendr/issues/19#issuecomment-758295860. As a result, an incorrect linker is pulled from the PATH, which results in cannot find -lgcc_eh error when building rust library. I have opened an issue https://github.com/r-lib/actions/issues/228 to see if we can get help with this, as I believe setup-r is misconfigured. The current solution is to rewrite paths, throwing away the ones matching rtools and mingw, like the following:

$temp = $env:Path -split [System.IO.Path]::PathSeparator | ? {$_ -notlike "*rtools*mingw*"} | join-string -Separator $([System.IO.Path]::PathSeparator)
echo $temp | Out-File -FilePath $env:GITHUB_PATH -Encoding utf8
yutannihilation commented 3 years ago

How about overriding PATH variable on rextendr's side? Probably we only need Rtools' toolchain and cargo. Here's a quick example: https://github.com/yutannihilation/rextendr/commit/11009361d0672bb26bc5f7fa8987e119f5b202f7

I mean, I'm wondering if it's a common case not only for setup-r action but also for ordinary Windows users to misconfigure PATH with another MinGW toolsets, which might be bundled with various tools.

Ilia-Kosenkov commented 3 years ago

@yutannihilation, The PATH problem is unique to the Github Actions we use. There is no need to fix it in code as regular users with correctly installed rtools will not experience it.

In fact, my issues has already been addressed and closed (https://github.com/r-lib/actions/commit/1efbdeb0f82823ee4538248d30b66ef7b38990ed), I will try this modified CI step in my script.

yutannihilation commented 3 years ago

regular users with correctly installed rtools will not experience it.

OK, sounds fair. I probably thought of those who "incorrectly" installed rtools, but they need to do it right to compile packages anyway.

my issues has already been addressed and closed

So quick!

Ilia-Kosenkov commented 3 years ago

@yutannihilation, Unfortunately it appears to be bugged because the author of the fix put string value in the if condition. So basically trying to check if ("true") or if ("false"), which appears to be always true. I am trying to fix it in my fork and reopen that issue.

Ilia-Kosenkov commented 3 years ago

People @ r-lib fixed rtools path issue in https://github.com/r-lib/actions/commit/30e0be01ce01d521431906cf82be954edbc3e51d, which I have tested in my CI.

Notice however that to get latest updates the correct action is r-lib/actions/setup-r@master, not r-lib/actions/setup-r@v1.

clauswilke commented 3 years ago

If you end up using the master branch of setup-r, please open an issue to remind us to change this to a specific version once it's released. I don't necessarily want to depend on master for the foreseeable future.

Ilia-Kosenkov commented 3 years ago

Reopening again ... The problem with the linker turned out to be severe. Because the majority of Windows users run R x64 and default rust toolchain is also x64 (stable-x86_64-pc-windows-gnu), there is no problem in building rust library targeting x86_64-pc-windows-gnu. However, if the same toolhcain is used to build 32-bit (i.e. for i686-pc-windows-gnu), build fails with missing linker error.

I uncovered this when setting up tests for rextendr and forcing multiarch to test both 32- and 64-bit.

The solution is, unfortunately, using stable-i686-pc-windows-gnu toolchain to build for i686-pc-windows-gnu.

At the same time, because stable-msvc relies on external linker (from msys2), if msys2 is on the path (both 32- and 64-bit), one toolchain can be used to build everything.

This creates a problem: on Windows, we need a way to understand what toolchain is asked for (e.g., stable-gnu) and inject correct toolchains into build command (in addition to currently injected --target).

Ilia-Kosenkov commented 3 years ago

Linking info on cross-compilation https://github.com/japaric/rust-cross

yutannihilation commented 3 years ago

(Sorry, I'm yet to figure out the problem...) Can it be fixed by explicitly specifying linker with [target] key?

c.f. https://stackoverflow.com/a/31492975

Ilia-Kosenkov commented 3 years ago

@yutannihilation, In principle it can, but to get i686 linker you need i686 gnu tool chain.

yutannihilation commented 3 years ago

I see.

I confirmed it errors on 32bit R on my laptop, and toolchain = "stable-i686-pc-windows-gnu" passes. So, what you are suggesting is

This creates a problem: on Windows, we need a way to understand what toolchain is asked for (e.g., stable-gnu) and inject correct toolchains into build command (in addition to currently injected --target).

something like checking .Platform$r_arch and choosing the appropriate toolchain name (stable-ARCH-pc-windows-gnu), right?

> rust_function("fn add(a:f64, b:f64) -> f64 { a + b }", toolchain = "stable-gnu")
build directory: C:\Users\HIROAK~1\AppData\Local\Temp\RtmpkBvUyN\file13d8576e9d6

    Updating git repository `https://github.com/extendr/extendr`
    Updating crates.io index
   Compiling winapi-build v0.1.1
   Compiling winapi-x86_64-pc-windows-gnu v0.4.0
   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.58
   Compiling extendr-engine v0.1.11 (https://github.com/extendr/extendr#f714309f)
   Compiling lazy_static v1.4.0
   Compiling kernel32-sys v0.2.2
   Compiling quote v1.0.8
   Compiling extendr-macros v0.1.11 (https://github.com/extendr/extendr#f714309f)
   Compiling libR-sys v0.2.1
   Compiling extendr-api v0.1.11 (https://github.com/extendr/extendr#f714309f)
   Compiling rextendr1 v0.0.1 (C:\Users\HIROAK~1\AppData\Local\Temp\RtmpkBvUyN\file13d8576e9d6)
error: linker `i686-w64-mingw32-gcc` not found
  |
  = note: 指定されたファイルが見つかりません。 (os error 2)

error: aborting due to previous error

error: could not compile `rextendr1`

To learn more, run the command again with --verbose.
Error: Rust code could not be compiled successfully. Aborting.
> rust_function("fn add(a:f64, b:f64) -> f64 { a + b }", toolchain = "stable-i686-pc-windows-gnu")
build directory: C:\Users\HIROAK~1\AppData\Local\Temp\RtmpkBvUyN\file13d8576e9d6

    Updating crates.io index
  Downloaded kernel32-sys v0.2.2
  Downloaded quote v1.0.8
  Downloaded winapi v0.2.8
  Downloaded libR-sys v0.2.1
  Downloaded winapi-build v0.1.1
  Downloaded syn v1.0.58
  Downloaded 6 crates (1.1 MB) in 12.30s
   Compiling winapi-i686-pc-windows-gnu v0.4.0
   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.58
   Compiling extendr-engine v0.1.11 (https://github.com/extendr/extendr#f714309f)
   Compiling lazy_static v1.4.0
   Compiling kernel32-sys v0.2.2
   Compiling quote v1.0.8
   Compiling extendr-macros v0.1.11 (https://github.com/extendr/extendr#f714309f)
   Compiling libR-sys v0.2.1
   Compiling extendr-api v0.1.11 (https://github.com/extendr/extendr#f714309f)
   Compiling rextendr2 v0.0.1 (C:\Users\HIROAK~1\AppData\Local\Temp\RtmpkBvUyN\file13d8576e9d6)
    Finished dev [unoptimized + debuginfo] target(s) in 6m 10s
Ilia-Kosenkov commented 3 years ago

@yutannihilation, This is the easiest solution I think. However, it would be nice to override toolchains. Perhaps we can expand how toolchain parameter is treated. If it is NULL, use gnu toolchains with appropriate architecture. If it is a string, use this toolchain for all builds (like I do with stable-msvc). If it is length() > 1 character vector, then use names to determine toolchain per architecture, e.g. c(`x86_64` = "stable-x86_64-gnu", `i686` = "stable-i686-gnu")

I just can't seem to come up with a good solution of how to take into account system default toolchain like it is used on all other systems.

yutannihilation commented 3 years ago

If it is length() > 1 character vector, then use names to determine toolchain per architecture, e.g. c(`x86_64` = "stable-x86_64-gnu", `i686` = "stable-i686-gnu")

Do we really need this complexity? The users don't usually compile the same Rust code on two different arch, except when they want to create an R package. In that case, they are supposed to use that two toolchains all the time. Sorry, probably I don't get what problem you are addressing yet...

Btw, I struggled to read cargo code. The reason of the error linker `i686-w64-mingw32-gcc` not found when stable-gnu (which seems to be called as "partial target triples") seems that, if we don't specify arch, rustup resolves it as the host's arch. It seems there's no option to inject the arch, so probably there's now way to expand it on R's side.

https://github.com/rust-lang/rustup/blob/0722f876d613e900e786f37ba802a44251461706/src/dist/dist.rs#L43-L54 https://github.com/rust-lang/rustup/blob/50fbbebed0a6c31c9ad75c0f56017a59771c9033/src/config.rs#L893 https://github.com/rust-lang/rustup/blob/0722f876d613e900e786f37ba802a44251461706/src/dist/dist.rs#L273-L314

Ilia-Kosenkov commented 3 years ago

@yutannihilation, I am not realy proficient enough to dig deep into rustup code. Without this complexity (or any other solution) we won't be able to run tests on rextendr itself if it will contain any tests at all. rextendr code can be put into test code of another package, whcih will again fail for i386.

I believe I may be able to provide a solution for this problem on R side, at least for the time being.

yutannihilation commented 3 years ago

we won't be able to run tests on rextendr itself if it will contain any tests at all.

I mean, we probably don't need to expose the complex interface to users just to pass tests with standard toolchains. If needed, the users can write their own if branches with .Platform$r_arch. I feel these two are enough.

If it is NULL, use gnu toolchains with appropriate architecture. If it is a string, use this toolchain for all builds (like I do with stable-msvc).

Ilia-Kosenkov commented 3 years ago

@yutannihilation, Ok, after we merge #25, I will try to implement default gnu toolchain for Windows.

Ilia-Kosenkov commented 3 years ago

@clauswilke in discussion of #26 made a good point that we should have consistent tools throughout *extendr projects. So far, because of problems with rust gnu toolchain, we use everywhere stable-msvc and rely on MSYS2. This is acually a good and universal setup that allows building for both architectures, and it also supports binding generation of libR-sys.

With this in mind, rextendr will rely on default toolchain (which is expected to be *-msvc). A user can override this behavior by specifying toolchain. As @yutannihilation suggested, it will be up to the user to provide correct toolchain for each architecture.

Right now CI uses stable-msvc https://github.com/extendr/rextendr/blob/718812de9d041dce046f4a65dcd0ebeccd59b9ca/.github/workflows/R-CMD-check.yaml#L19-L26 And setts up MSYS2 paths

https://github.com/extendr/rextendr/blob/718812de9d041dce046f4a65dcd0ebeccd59b9ca/.github/workflows/R-CMD-check.yaml#L67-L74

Which allows rextendr to pass rcmdcheck with --force-multiarch.

yutannihilation commented 3 years ago

@Ilia-Kosenkov Thanks for investigation and wrapping up! At first, I slightly preferred gnu toolchains as it seemed a bit heavy to install MSYS2, but I now agree with you on this.

This is acually a good and universal setup that allows building for both architectures, and it also supports binding generation of libR-sys.

Probably we should require installing MSYS2 at the moment in case we ask them to try compilation w/ bindgen (and I expect the ordinary Windows users who just install the packages can use pre-compiled binaries c.f. https://github.com/extendr/extendr/issues/51).

@clauswilke @CGMossa IIUC, now we can simply

tell people to always make msvc the default.

Can we close this issue? Are there any points on this topic that we still need to discuss?

clauswilke commented 3 years ago

Sounds good. I think the issue can be closed. Are the installation instructions for Windows here correct or not?

yutannihilation commented 3 years ago

I think the instructions are mostly correct. One thing, users need to define MSYS_ROOT by themselves (c.f. https://github.com/extendr/libR-sys/#windows-specific-instructions, this is because MSYS2 can be installed by various ways thus the root varies).

clauswilke commented 3 years ago

Thanks. For simplicity, I just removed MSYS_ROOT entirely from the instructions. I don't think it's needed for rextendr. We're not using it in the GH Action scripts.

yutannihilation commented 3 years ago

Looks good, but I guess you probably meant msys2 instead of msys32...?

https://github.com/extendr/rextendr/pull/27

clauswilke commented 3 years ago

I did mean msys32, but maybe msys64 is better.

The screenshot on the msys2 page uses msys32: https://www.msys2.org/#installation

But these days, we normally use 64-bit systems, which would go into msys64: https://www.msys2.org/wiki/MSYS2-installation/

clauswilke commented 3 years ago

There's one thing I still don't understand: If we don't compile libR-sys with bindgen, why can't we use the toolchain that comes with Rtools even when using stable-msvc, if in the end we're using mingw linkers anyway? Is this a version incompatibility, or would we have to install other packages through Rtools?

Ilia-Kosenkov commented 3 years ago

@clauswilke, There is a mismatch in gcc versions, at least. Rtools ships 8.3.0, msys2 has 10.2.0. Your build will fail with linker error of missing gcc_eh library, which is not present in 8.3.0. I was actually experimenting with this today. Essentially, rtools is modified msys2, so a user needs two nearly identical subsystems installed simultaneously. We cannot install newer gcc onto rtools because packages rely on 8.3.0.

Now I am not sure from where this gcc_eh library requirement comes from. If we can find out, we can probably solve this problem with rtools only.

clauswilke commented 3 years ago

Is gcc_eh not present in gcc 8.3 or not present in Rtools? gcc 8.3 is only two years old, it's not that unreasonable to ship it in my opinion. gcc_eh is definitely older. Maybe it's a problem with how Rtools sets up their compiler? I know they're setting up everything to link statically, maybe that's what's causing the issue. (Though gcc_eh is needed for static builds, so that's confusing to me.)

Ilia-Kosenkov commented 3 years ago

@clauswilke, I cannot say. msys2's package manager does not seem to support package versioning (or, rather, repositories have only current version of a package), so I tried building gcc 8.3.0 using PKGBUILD script from mingw repository, but it unfortunately errored while building gcc from source, and I am not sure why.

I think we should somehow install older version of mingw64 toolchain, or just older gcc. If it has gcc_eh, then it is rtools. If it does not, then it is older version of gcc or some rust configuration (perhaps rust requires newer gcc).

gcc_eh is passed as -lgcc_eh to rustc, so I doubt we have any power over it.

clauswilke commented 3 years ago

Here is the explanation, from this old, random bug report:

Note that if enabled_shared is set to yes, then libgcc-eh is built; otherwise, it is added to libgcc. And this confirms what I am seeing, which is that gcc built with --disable-shared does not produce a libgcc_eh.a (totally weird that if shared is enabled, libgcc_eh.a, which is not a shared library, does get built).

Rtools builds with --disabled-shared: https://github.com/r-windows/rtools-packages/blob/df6b4363b0422db30abf7dd78378022666a24ab4/mingw-w64-gcc/PKGBUILD#L167

msys2 builds with --enable-shared: https://github.com/msys2/MINGW-packages/blob/950bcf391e5b247ebc7f2b2f5f5d10a03c3fdd63/mingw-w64-gcc/PKGBUILD#L223

This is fundamentally a gcc bug, and in particular when cross-compiling. See e.g. this old message from some development mailing list: https://sourceware.org/legacy-ml/libc-alpha/2003-09/msg00100.html

Rtools could fix it by building with --enable-shared, but I guess one of their main points is that they don't want to allow shared builds.

Ilia-Kosenkov commented 3 years ago

@clauswilke, I tried building mingw-w64-gcc from rtools-packages after replacing --disabled-shared with --enabled-shared, but ultimately failed with the strangest error:

.../PKGBUILD: line 168: --enable-shared: command not found

It looks like it was removed/disabled on purpose.