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

Fix compile error on Windows with Rust 1.70 #285

Closed yutannihilation closed 1 year ago

yutannihilation commented 1 year ago

Fix https://github.com/extendr/extendr/issues/559

yutannihilation commented 1 year ago

Thanks, I wrote some explanation on https://github.com/extendr/extendr/issues/559.

But I'm yet to figure out what this indicates.

 * checking whether package 'testpkg' can be installed ... [20s] WARNING
Found the following significant warnings:
  Warning: corrupt .drectve at end of def file
yutannihilation commented 1 year ago

It seems it indicates some incompatibility between compilers.

https://stackoverflow.com/questions/25161814/warning-corrupt-drectve-at-end-of-def-file

There are two possibilities:

  1. LLVM (Rust's codegen backend) and Rtools' GCC are incompatible
  2. Rust's MSVC toolchain and GNU target are incompatible

I think 2. is unlikely. If it were true, things should break more badly. So I guess 1 is the cause. LLVM was upgraded to version 16 in Rust 1.70.

https://github.com/rust-lang/rust/pull/109474

The attempt to upgrade LLVM to version 16 failed once for some reason. We may be able to find some context in this lengthy thread (I mean, I don't read this yet...).

https://github.com/rust-lang/rust/pull/107224

yutannihilation commented 1 year ago

We may be able to find some context in this lengthy thread

Hmm, no. https://github.com/rust-lang/rust/pull/109474 says it was "ABI-incompatibility between libstdc++ 7 and 8," so it seems unrelated.

yutannihilation commented 1 year ago

This one?

https://github.com/llvm/llvm-project/commit/c5b3de6745c37dd991430b9b88ff97c35b6fc455

yutannihilation commented 1 year ago

Minimal reproducible example

https://github.com/yutannihilation/rust170_gnu_warning

Ilia-Kosenkov commented 1 year ago

Looks like an LLVM thing that Yutani linked above. Should we raid llvm issues and ask there?

yutannihilation commented 1 year ago

Thanks, I think this is primarily Rust's issue. I'll report this to Rust's repo after waiting for a while to get answers in the user forum.

https://users.rust-lang.org/t/corrupt-drectve-warning-on-x86-64-pc-windows-gnu-target-with-rust-1-70/94912

I'd appreciate if you ask to LLVM's side!

Note that, I'm afraid this warning won't be resolved because this is what we can just ignore, no real harm, iiuc. But it probably means Rust package will be kicked out from CRAN...

eitsupi commented 1 year ago

But it probably means Rust package will be kicked out from CRAN...

Hopefully the version will be skipped as Rust on Windows in CRAN does not seem to be updated frequently......

yutannihilation commented 1 year ago

Yeah, but Jeroen said CRAN already used Rust 1.69, which was the latest at the moment, so I'm not sure if we can be that optimistic.

https://github.com/extendr/rextendr/issues/279#issuecomment-1555904537

yutannihilation commented 1 year ago

Reported: https://github.com/rust-lang/rust/issues/112368

yutannihilation commented 1 year ago

@CGMossa @Ilia-Kosenkov I'm merging this while this doesn't actually "fix" the warning on oldrel because I'm afraid there's no way to fix it actually. Please propose alternatives if you come up with a good idea.

eitsupi commented 1 year ago

Since this change seems important, could you please do a new patch release? It has been over a week since the last release so it should be allowed......

Ilia-Kosenkov commented 1 year ago

Is this change so important? We hard-coded a dependency which is only needed if you run Rust 1.70 during compilation. @yutannihilation, @CGMossa, what do you think?

yutannihilation commented 1 year ago

I think it would be frustrating especially for new users that their first R package using rextendr::use_extendr() won't work. That said, existing users won't be saved anyway, and ... probably a week is too frequent? CRAN Repository Policy says

Submitting updates should be done responsibly and with respect for the volunteers’ time. Once a package is established (which may take several rounds), “no more than every 1–2 months” seems appropriate.

yutannihilation commented 1 year ago

Is this change so important?

To be clear, I think this compilation error is a critical problem. That's why I tried so hard to investigate the cause and find the fix as soon as possible. What I'm not sure is how important this rextendr's template is. I use my own version of Makevars.win etc rather than the ones rextendr provides, so I think I'm not the right person to decide.

CGMossa commented 1 year ago

I simply hate any friction that may occur with fresh new users. While I think we should issue a patch, it could be that we should give it a week or so, just go be on the safe side.

But we cannot have a an issue for those who install latest r, latest rtools, latest rust..

eitsupi commented 1 year ago

probably a week is too frequent?

Just for information, I have been told that R actually checks to see if it is within a week of the last release and if there have been less than 6 releases in the past 6 months.

yutannihilation commented 1 year ago

Oh, I didn't know this, thanks.

Ilia-Kosenkov commented 1 year ago

Funny that in the modern era of CI CD you have 6 releases per year on CRAN. I suggest we wait a week and I will make a patch release with a comment that we updated templates to align with latest rust release

eitsupi commented 1 year ago

@Ilia-Kosenkov Now that a week has passed, perhaps a new release can be made? Thanks.

eitsupi commented 1 year ago

Since Rust on GitHub Actions is currently 1.70 by default, this problem occurs.

yutannihilation commented 1 year ago

@eitsupi Just curious. Why is the CRAN release important to you? You need to fix the Makevars.win in your existing packages by yourself anyway. In particular, I'm wondering why you haven't fix prqlr yet. Is it related to how you set up GHA CI?

eitsupi commented 1 year ago

Just curious. Why does the CRAN release important to you? You need to fix the Makevars.win in your existing packages by yourself anyway. In particular, I'm wondering why you haven't fix prqlr yet. Is it related to how you set up GHA CI?

Simply because I myself suffered a lot from extendr's (at the time) existing templates not working.

yutannihilation commented 1 year ago

I see. I agree it's really frustrating.

Ilia-Kosenkov commented 1 year ago

I will do this on Monday if that is OK for you.