-
Notifications
You must be signed in to change notification settings - Fork 27
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
On Windows, default rust needs to be gnu, not msvc #19
Comments
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. |
I can offer this workaround, that I think is fairly okay to use..
Even if the default is Although, I'm not sure Loosely, the build commands would then be like #...
status <- system2(
command = "rustup",
args = c(
"run",
"stable-gnu",
"cargo",
"build",
... |
@CGMossa |
I did share it, see the link that Claus posted, or here:
While I'm at it: Using |
Oh, I see. Sorry. I thought you got some different error after you added the target. Hmm... |
Before implementing some workaround I'd like to understand what the issue is. We're just calling |
Ok. I added
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:
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. |
Thanks. So, maybe we came back to this issue...? rust-lang/cargo#8990 (comment) (Sorry, I don't follow the discussion around this topic during the last year-end... It needs some time for me to catch up.) |
|
In the extendr github action scripts, we're explicitly adding We may need something similar here. You can set environment variables in |
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 Alternatively, we could mimic the approach of building an R package, that is, use |
I am sorry I fail to understand what actually triggered the linker error? Was it some simple test code? 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 Then it should work. And if it does not, please provide a minimal reproducible example and detailed output. |
Hmm..., I think I followed your instruction, but it failed.
|
@yutannihilation, |
Thanks, confirmed it works (In my environment, the path seems
|
@yutannihilation,
In both cases we need to provide So, my suggestion is to allow for |
@Ilia-Kosenkov Feel free to implement the Also, it would be great if you could add brief instructions on how to get things going on Windows here: |
@clauswilke, |
Thanks! I now understand what you meant. Confirmed #20 works on my environment. Without
|
I am reopnening this issue because I am unable to setup simple Windows CI using |
The root cause is that $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 |
How about overriding I mean, I'm wondering if it's a common case not only for |
@yutannihilation, In fact, my issues has already been addressed and closed (r-lib/actions@1efbdeb), I will try this modified CI step in my script. |
OK, sounds fair. I probably thought of those who "incorrectly" installed
So quick! |
@yutannihilation, |
People @ r-lib fixed rtools path issue in r-lib/actions@30e0be0, which I have tested in my CI. Notice however that to get latest updates the correct action is |
@clauswilke in discussion of #26 made a good point that we should have consistent tools throughout With this in mind, Right now CI uses rextendr/.github/workflows/R-CMD-check.yaml Lines 19 to 26 in 718812d
And setts up MSYS2 paths
rextendr/.github/workflows/R-CMD-check.yaml Lines 67 to 74 in 718812d
Which allows |
@Ilia-Kosenkov
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. extendr/extendr#51). @clauswilke @CGMossa
Can we close this issue? Are there any points on this topic that we still need to discuss? |
Sounds good. I think the issue can be closed. Are the installation instructions for Windows here correct or not? |
I think the instructions are mostly correct. One thing, users need to define |
Thanks. For simplicity, I just removed |
Looks good, but I guess you probably meant |
I did mean The screenshot on the msys2 page uses But these days, we normally use 64-bit systems, which would go into |
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? |
@clauswilke, Now I am not sure from where this |
Is |
@clauswilke, I think we should somehow install older version of mingw64 toolchain, or just older
|
Here is the explanation, from this old, random bug report:
Rtools builds with msys2 builds with This is fundamentally a gcc bug, and in particular when cross-compiling. See e.g. this old message from some development mailing list: Rtools could fix it by building with |
@clauswilke,
It looks like it was removed/disabled on purpose. |
@clauswilke, |
Another observation in rtools, ming32 version of gcc contains I think this is possible because from what @clauswilke wrote, gcc compiles everything into I think we should ask rtools people to implement this bypass for mingw64 as well. Here is proof that ("C:\rtools40\mingw32\lib\gcc\i686-w64-mingw32\8.3.0\libgcc.a", "C:\rtools40\mingw32\lib\gcc\i686-w64-mingw32\8.3.0\libgcc_eh.a")
| Get-FileHash Algorithm Hash Path
--------- ---- ----
SHA256 621AEF2A5AD649F24D17081657F611F9DFB20E2E26B74B30F7D5756A00DD9217 C:\rtools40\mingw32\lib\gcc\i686-w64-mingw32\8.3.0\libgcc.a
SHA256 621AEF2A5AD649F24D17081657F611F9DFB20E2E26B74B30F7D5756A00DD9217 C:\rtools40\mingw32\lib\gcc\i686-w64-mingw32\8.3.0\libgcc_eh.a |
And here is the commit that introduced that hack, apparently The comment says this hack is for |
This is a great find! Could you open an issue about this with rtools-packages, and maybe also a PR? (I would always do an issue first if you're not a frequent contributor to a project, even if the PR is straightforward.) |
I can confirm that after implementing this fix for mingw64
creating a new gcc package using EDIT01: Note that building package from source took more than an hour on a decent desktop. |
Can we close this issue? |
Yes, we decided to go with |
Apparently, under some circumstances, the default Rust on Windows needs to be gnu, not msvc.
See:
#15 (comment)
and
#7 (comment)
Is there any way to remove this requirement?
cc @Ilia-Kosenkov @CGMossa @yutannihilation
The text was updated successfully, but these errors were encountered: