r-universe-org / help

Support and bug tracker for R-universe
https://docs.r-universe.dev/
8 stars 2 forks source link

Install GNU targets of Rust toolchain? #219

Closed yutannihilation closed 1 year ago

yutannihilation commented 1 year ago

I'm surprised that this is accepted by CRAN. In my understanding, rustup target add should modify ~/.cargo, which is not allowed by the CRAN repository policy. The reason why I guess this didn't get the CRAN maintainers angry is probably the targets are already installed (not confirmed yet). So, maybe R-universe can follow the setup?

hellorust's Makevars.win: https://github.com/cran/hellorust/blob/f13573edd9600d4ae6d0760fca3826f53ddd9657/src/Makevars.win#L16-L17

jeroen commented 1 year ago

R-universe builds on GitHub actions, so we are based on that setup. If we would know if a good way to test if a package needs rust, we could perhaps automatically install the toolchain on r-universe for those packages.

The line in hellorust is there to make it work on non-cran hosts, in particular CI systems such as github-actions, that do have rust/cargo installed but default to MSVC.

The CRAN toolchains default to gnu mingw toolchains so that line does not change anything, it just prints a confirmation that the compiler is available. I suppose you can comment-out the line for a CRAN submission if you want to be really careful, because it is not needed on CRAN, though I think it is harmless.

yutannihilation commented 1 year ago

Thanks.

If we would know if a good way to test if a package needs rust, we could perhaps automatically install the toolchain on r-universe for those packages.

I'm sure it's good enough to check if DESCRIPTION has cargo for SystemRequirements. But, okay, I get your point. I thought R-universe's setup is the same as CRAN, which is kept secret, and hellorust's target is only CRAN and R-universe. To make it work generally, executing rustup might be fair.

jeroen commented 1 year ago

I'm sure it's good enough to check if DESCRIPTION has cargo for SystemRequirements.

The problem is that the package may also get built as a dependency of another package. So this means that whenver we install any R package, we would need to download and inspect all DESCRIPTION files of all dependencies to look for cargo in the SystemRequirements, before starting the build. This would be quite expensive en fragile.

yutannihilation commented 1 year ago

Ah, I see. The problem seems more complex than I expected. To avoid the fragility, you can choose to always install the targets, but it would be too expensive considering there are only several Rust packages on CRAN.

jeroen commented 1 year ago

I made a pull request to the GHA image to preinstall the target: https://github.com/actions/runner-images/pull/6661

yutannihilation commented 1 year ago

Yay, it got merged! Thanks so much.

eitsupi commented 1 year ago

Does it seem to be failing to build despite that runner's update was released? cc @paithiov909 https://github.com/r-universe/paithiov909/actions/runs/3685270724/jobs/6236133928#step:9:58

jeroen commented 1 year ago

I just tested this with hellorust, and it seems to work: https://github.com/r-rust/hellorust/commit/822c1681cecbcb8b2f2e10c343b37f7cce3ab057

I did notice you use a target stable-x86_64-pc-windows-gnu and we use x86_64-pc-windows-gnu so perhaps that matters?

yutannihilation commented 1 year ago

As discussed on another place, extendr uses GNU toolchain, not the default MSVC toolchain, on Windows with R >= 4.2. So, this difference might matter.

Note that, on R 4.1, R tries to build the 32-bit version, so you need to install i686-pc-windows-gnu target (I personally recommend dropping the support for R 4.1.).

eitsupi commented 1 year ago

FYI, I had to add the following line, which is slightly different from hellorust, to get a successful build of a package that uses extendr on GitHub Actions in eitsupi/prqlr#5

rustup:
    rustup toolchain install stable-gnu || true

I don't know what this little difference in names indicates to me...

jeroen commented 1 year ago

I think if you just remove the +$(TOOLCHAIN) from your Makevars.win and only specify gnu in the --target=$(TARGET) then it should work with the preinstalled rust on GitHub actions.

eitsupi commented 1 year ago

I think if you just remove the +$(TOOLCHAIN) from your Makevars.win and only specify target=x86_64-pc-windows-gnu then it should work with the preinstalled rust on GitHub acitons?

Thank you for your suggestion. I tryed in eitsupi/prqlr#22, but it seems failed.

Sorry, it's my typo... (missing --)

In eitsupi/prqlr#22, the build seems to have succeeded with this change! Perhaps in exchange for R 4.1 support on Windows it seems to be possible to get a successful build on GitHub without a hack?

yutannihilation commented 1 year ago

Thanks so much for exploring! I think extendr can switch back to the MSVC toolchain.

jeroen commented 1 year ago

@yutannihilation I think that is a good idea. To be more specific: we should just use the default toolchain that is available (i.e. not specify any +... parameter) and only specify the --target, because that is what really matters.

yutannihilation commented 1 year ago

I want to agree with you, but the toolchain also matters (or mattered, at least). I think I'm not good enough to figure out what the things are and should be, but I'd like you to know we actually had several reasons to specify the toolchain; the other toolchain didn't worked for the 32-bit Windows, and the cross compilation has some limitation (e.g. doc tests are not executed).

eitsupi commented 1 year ago

Sorry to continue this thread again, but after switching toolchain, Windows builds of prqlr on R-universe now fail. Normal GitHub Actions are fine, so I am wondering where the difference is.

error: linking with link.exe failed: exit code: 1

https://github.com/r-universe/eitsupi/actions/runs/3741799364/jobs/6361839313#step:9:161

yutannihilation commented 1 year ago

Curious. I don't see such a problem on my package.

https://github.com/r-universe/yutannihilation/actions/runs/3720180814/jobs/6309514049

yutannihilation commented 1 year ago

These look the same error (link: missing operand after '\377\376"'), but I couldn't find what's the cause.

yutannihilation commented 1 year ago

One difference might be that I'm using the dev version of the libR-sys crate, which removed the dependency on the winapi crate. I don't remember well, but I think I have experienced some weird compilation error with it. Anyway, I'm curious why it fails only on R-universe's Windows runners while it works fine on the usual GHA runner (c.f. https://github.com/eitsupi/prqlr/actions).