search

extendr / rextendr

An R package that helps scaffolding extendr-enabled packages or compiling Rust code dynamically
https://extendr.github.io/rextendr/
Other
175 stars 25 forks source link

libgcc_eh.a hack no longer needed? #279

Closed jeroen closed 1 year ago

jeroen commented 1 year ago

I think the latest versions of rust on Windows no longer link to -lgcc_eh. I just released a version of gifski to CRAN that no longer has the libgcc_eh.a stub and all seems OK.

Can you confirm if you find the same?

Ilia-Kosenkov commented 1 year ago

Hi! Funny thing I am struggling with this right now. We unfortunately still rely on this, and new version of Rtools (42 & 43) does not have it, so we rely on hacks. What did you do to solve it?

jeroen commented 1 year ago

I upgraded rust to the latest version on windows, afaict, it no longer needs it. But perhaps something in my package stack changed as well, I'm not sure.

Ilia-Kosenkov commented 1 year ago

Can you be more specific on your setup? Which version of rtools do you use, which toolchain on windows and which target?

Ilia-Kosenkov commented 1 year ago

We currently do something like this https://github.com/extendr/rextendr/blob/f8f964ba268f4330188ddeb4255b1db4b676eedb/inst/templates/Makevars.win#L22

Ilia-Kosenkov commented 1 year ago

Also summoning @yutannihilation & @CGMossa since you both worked with this issue before.

jeroen commented 1 year ago

I removed the stub in https://github.com/r-rust/gifski/commit/153c92f3367d28d41883f72d822edd10db2de0eb. You can see from the CI that it works with rtools43 and the latest rust. CRAN on Windows has Rust 1.69.0 now I think.

Ilia-Kosenkov commented 1 year ago

I am not sure I follow, but this looks to me like you are using Rtools40.

ADDPATH=$(subst C:\,/c/,$(RTOOLS40_HOME))/mingw$(WIN)/bin:$(USERPROFILE)\.cargo\bin export CARGO_TARGET_X86_64_PC_WINDOWS_GNU_LINKER=$(RTOOLS40_HOME)\mingw64\bin\gcc export CARGO_TARGET_I686_PC_WINDOWS_GNU_LINKER=$(RTOOLS40_HOME)\\mingw32\bin\gcc

We use this linker https://github.com/extendr/rextendr/blob/f8f964ba268f4330188ddeb4255b1db4b676eedb/inst/templates/Makevars.ucrt#L3

Don't get me wrong, I would be so happy if we could drop this hack, but right now I do not understand how.

jeroen commented 1 year ago

I am not sure I follow, but this looks to me like you are using Rtools40.

This part is only used on < R 4.2.

Don't get me wrong, I would be so happy if we could drop this hack, but right now I do not understand how.

I think you don't have to do anything, afaict, the latest rust simply no longer hardcodes -lgcc_eh as it did before.

Ilia-Kosenkov commented 1 year ago

Ok, I see. I will have to check this manually on our test projects. I hope it is so, this hack is painful to maintain. I will get back to you with results, thanks for letting us know!

yutannihilation commented 1 year ago

Thanks for sharing, Jeroen. Just for reference, it seems rustc itself still uses libgcc_eh (and libgcc_s).

https://github.com/rust-lang/rust/blob/1b67f8b013890fec98e4a2e72b568731d3a58c1f/compiler/rustc_target/src/spec/windows_gnu_base.rs#L64

yutannihilation commented 1 year ago

I confirmed it works without libgcc_eh.a at least on my local and on GHA.

https://github.com/yutannihilation/string2path/pull/44

yutannihilation commented 1 year ago

It seems to work regardless of the version of Rust toolchain. So, if something was changed, it's probably on R's side? I don't setup the CI very carefully, so I might be wrong here.

https://github.com/yutannihilation/string2path/actions/runs/5036028906

yutannihilation commented 1 year ago

Oh, extendr fails. Curious.

https://github.com/extendr/extendr/actions/runs/5036073371/jobs/9031977734

yutannihilation commented 1 year ago

The failure happens on the test step, not on the build step. This is probably because cargo test requires unwinding (edit: Sorry, let me withdraw this part. Something must be different, but I don't know what it is.), whereas cargo build is not. So, I think this won't be a problem if the R package author tests only on R's side. If the author wants to write unit tests on Rust's side, they have to separate the crate (or use the fake libgcc_eh.a).

jeroen commented 1 year ago

It seems to be caused by setting a custom linker. If I use this I get the -lgcc_eh error:

CARGO_TARGET_X86_64_PC_WINDOWS_GNU_LINKER=gcc

However if I comment this out, the problem disappears. Setting CARGO_LINKER does not seem to have any effect at all.

Perhaps we should just set the PATH correctly to the toolchain and not override the linker. R 4.2 and up automatically set the PATH so it might just work.

yutannihilation commented 1 year ago

Yeah, you know, not using the Rtools' toolchain stops the error. But, I think that's not what we want.

yutannihilation commented 1 year ago

To be clear, I think you will argue it works fine in most of the cases. I can probably agree with you, but extendr has some reasons.

https://github.com/extendr/extendr/blob/ca001033e371ff3f52022cea72aa534f2958f041/extendr-api/src/graphics/device_driver.rs#L701-L723

jeroen commented 1 year ago

not using the Rtools' toolchain stops the error. But, I think that's not what we want.

I don't understand what you mean. We still set the PATH such that gcc is used from rtools, otherwise the embedded C code would not work in the first place. We just use the default linker settings?

Either way this is probably gone off topic, I am happy to have found that the -lgcc_eh problem disappears if we stick with the default cargo linker settings, which has become easier now that we only supports a single architecture, and R puts the toolchain on the PATH by default.

yutannihilation commented 1 year ago

Oh, you didn't know this? Then, sorry. The default linker name of the GNU target is "x86_64-w64-mingw32-gcc" and Rtools doesn't contain the same name of the GCC executable.

https://github.com/rust-lang/rust/blob/06345574d96da7c471ee59c991ce14f33741c0ee/compiler/rustc_target/src/spec/x86_64_pc_windows_gnu.rs#L13