-
Notifications
You must be signed in to change notification settings - Fork 24
Using rtools to test precomputed bindings on Windows #51
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
Conversation
9feff12
to
5b52d21
Compare
I think it's good to know that this works, but I'm not sure I would merge this at this time. We should just reach out to the Rtools maintainers and ask them to include this fix. I wouldn't want to tell our endusers that they need to manually copy library files (it will look scary to them) and I think our GH Actions should mirror the installation protocol we recommend in our manuals. |
@clauswilke, |
I'd also be Ok with adding one runner that does it this way, but without deleting the msys2-based runners. This way we can keep an eye on both options until Rtools is fixed. |
.github/workflows/test.yml
Outdated
cargo build -vv --features use-bindgen $(if ($target -ne 'default') {"--target=$target"} ) | ||
echo "::endgroup::" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't the endgroup statement have to come after the if
statement checking for errors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put "groups" around cargo
commands so log under spoiler contains only build/test related output.
If if
condition is met and error is thrown, it will be visible separtely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but see: extendr/extendr#130 (comment)
Does this not apply here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see, that is indeed very stupid of me. I will fix it with the next commit.
I added a copy of the workflow with a reduced number of steps to test rtools build as a separate job, restored the original workflow and added |
if: runner.os == 'Windows' | ||
# 1. Configure path to libclang | ||
# 2. Add path to mingw32/mingw64 -- otherwise library is linked to rtools | ||
# 3. Add path to R's i386/x64 -- to solve x86 build/test issue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, missed this earlier: The comments look like they need updating.
} | ||
|
||
|
||
# Run tests again using different bindings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here: comment looks wrong.
* Reordered stepps * Grouped logging * Setting up rtools path * Amending libgcc_eh for rtools * Setting up MSYS2 for binding generation on Windows * Cleanup * Setting rtools as a separate job * Restoring log groups * ::endgroup:: no longer swallows exit status of cargo
After discussing this issue in extendr/rextendr#19, a solution was found to build & test
libR-sys
using only rtools. This does not work for bindings generation (no clang provided with rtools), but is suitable for user consumption.Currently, rtools mingw64 should be "fixed" (see https://github.com/r-windows/rtools-packages/blob/master/mingw-w64-gcc/PKGBUILD#L305-L309) by duplicating
libgcc.a
aslibgcc_eh.a
, which satisfiesrustc
and linker.Build & test were reordered.
First, rtools is configured and precomputed bindings are tested.
Second, msys2 is enabled and bindings generation is tested using msys2.
This does not affect other systems.
Partially addresses #21.