Skip to content
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

msys2 is no longer needed on Windows? #91

Closed
Ilia-Kosenkov opened this issue Mar 19, 2021 · 15 comments · Fixed by #117
Closed

msys2 is no longer needed on Windows? #91

Ilia-Kosenkov opened this issue Mar 19, 2021 · 15 comments · Fixed by #117

Comments

@Ilia-Kosenkov
Copy link
Member

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

rextendr/R/source.R

Lines 225 to 244 in 09c8305

# Append rtools path to the end of PATH on Windows
if (
isTRUE(use_rtools) &&
.Platform$OS.type == "windows" &&
nzchar(Sys.getenv("RTOOLS40_HOME"))
) {
env_path <- Sys.getenv("PATH")
# This retores PATH when function returns, i.e. after cargo finishes.
on.exit(Sys.setenv(PATH = env_path))
r_tools_path <-
normalizePath(
file.path(
Sys.getenv("RTOOLS40_HOME"), # {rextendr} targets R >= 4.0
paste0("mingw", ifelse(R.version$arch == "i386", "32", "64")),
"bin"
)
)
Sys.setenv(PATH = paste(env_path, r_tools_path, sep = .Platform$path.sep))
}

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

echo "C:\msys64\mingw64\bin" | Out-File -FilePath $env:GITHUB_PATH -Encoding utf8 -Append
echo "C:\msys64\mingw32\bin" | Out-File -FilePath $env:GITHUB_PATH -Encoding utf8 -Append

echo "C:\msys64\mingw64\bin" | Out-File -FilePath $env:GITHUB_PATH -Encoding utf8 -Append
echo "C:\msys64\mingw32\bin" | Out-File -FilePath $env:GITHUB_PATH -Encoding utf8 -Append

and these are outputs from two primary CIs:

@clauswilke
Copy link
Member

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
Copy link
Member Author

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
Copy link
Contributor

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

@CGMossa
Copy link
Member

CGMossa commented Mar 22, 2021 via email

@yutannihilation
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Contributor

@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
Copy link
Contributor

While the test on package generation works

I also have no idea why this succeeds...

@Ilia-Kosenkov
Copy link
Member Author

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
Copy link
Contributor

yutannihilation commented Mar 27, 2021

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
Copy link
Member Author

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
Copy link
Contributor

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

@clauswilke
Copy link
Member

Can we close this issue now?

@Ilia-Kosenkov
Copy link
Member Author

Can we close this issue now?

Not 100% sure, we still include msys here:

echo "C:\msys64\mingw64\bin" | Out-File -FilePath $env:GITHUB_PATH -Encoding utf8 -Append
echo "C:\msys64\mingw32\bin" | Out-File -FilePath $env:GITHUB_PATH -Encoding utf8 -Append

I am going to look into this to make sure that we need only Rtools40v2 to make it work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants