Update Github Actions for Windows #34

Ilia-Kosenkov · 2020-12-21T12:08:27Z

Using insights from #9 we can now more or less reliably generate bindings for both 32- and 64-bit Windows.
With the path trick discussed in #29 i386/i686 tests can be run (though most of the tests are disabled).

I suggest to split Windows job into two separate ones and build bindings separately for each architecture.
I am not sure if I correctly copied the binding upload process, this should be checked.

clauswilke · 2020-12-21T14:17:26Z

Thanks for your efforts on this one. I have two comments:

1. I'm not a big fan of breaking the windows builds into two separate runs because most of their time is spent setting up the R and Rust environments, so there's a lot of wasted compute. I know we get the GH Action hours for free but we should still be cognizant of resource use.

2. The action scripts need to be completely reworked along the lines of the extendr scripts so we can target specific R versions:
https://github.com/extendr/extendr/blob/master/.github/workflows/test.yml
We will eventually need at least release, devel, and oldrel for all architectures, I think. See e.g. here:
https://github.com/r-lib/actions/blob/master/examples/README.md#tidyverse-ci-workflow

It may make sense to put everything into one single matrix or to have two matrices, one for unix-like builds and one for windows builds. Not sure about that one, depends on how much special casing is needed.

If you'd like to take a stab at this, you're welcome to do so.

clauswilke · 2020-12-21T14:20:33Z

To be clear: It's the following part of the extendr test script that would make a good starting point for libR-sys:
https://github.com/extendr/extendr/blob/d27585f43f900c49f7a805be6ce3cd83a196ab6d/.github/workflows/test.yml#L112-L149
We don't need to install and cache R packages.

Ilia-Kosenkov · 2020-12-21T14:30:47Z

@clauswilke ,
I was thinking about merging everything into a single job, but decided to progress in small steps.

I can try to rewrite script following extendras an example. If I fail or someone else does this faster, we can safely discard this PR.

Ilia-Kosenkov · 2020-12-21T20:46:31Z

So this is a template for a new GH Actions script, inspired by extendr.
There are two things to address: first, ubuntu::r-devel fails tests, which I presume also happens for other repos and is not our fault; second -- I am not sure how bindings and artifacts are now generated -- before we had one job for each os, now -- multiple. Perhaps there should be some condition on which to generate bindings -- for instance, only for rust-stable.

clauswilke · 2020-12-21T20:55:57Z

To make tests work on r-devel you need to set LD_LIBRARY_PATH, see here:
https://github.com/extendr/extendr/blob/06bd2a400894eb8fe9a43360afb6da1a1c8d7a30/.github/workflows/test.yml#L155-L156
(This is most likely the reason why you'll have to treat Unix and Windows separately.)

For the artifacts, there's nothing to worry about, generating and saving the artifacts costs practically nothing. I think it's good to have artifacts for all settings we test. This allows us to check whether anything changes, e.g., whether bindings under nightly look different from those under stable.

clauswilke · 2020-12-21T20:58:05Z

.github/workflows/test.yml

-        uses: actions/upload-artifact@main
+          r-version: ${{ matrix.config.r }}
+
+      - name: Set up `rust`


I think this should be Rust instead of `rust`. In general, I think names of programming languages should not be set in backticks, but more specifically here I don't think the backticks are converted into code font.

clauswilke · 2020-12-21T20:58:28Z

.github/workflows/test.yml

-      - uses: actions/checkout@v2
-      - uses: msys2/setup-msys2@v2
+      # This step is only needed for Windows
+      - name: Set up `msys2` for Windows


I would similarly delete backticks here.

clauswilke · 2020-12-21T20:58:45Z

.github/workflows/test.yml


+      # Run tests again using different bindings
+      - name: Run tests on precomputed bindings shipped with libR-sys
+        run: cargo test -vv $env:CARGO_BUILD_FLAGS -- --nocapture --test-threads=1


Please add a newline at the end of the file.

Ilia-Kosenkov · 2020-12-21T21:01:58Z

I noticed I screwed up with matrix.config. I will check everything is working correctly and fix all issues.

Ilia-Kosenkov · 2020-12-21T23:27:21Z

Debugging GitHub actions is a real pain.
@clauswilke, I think I fixed everything you mentioned, take a look when you have time.

Should I squash commits? GHA requires a lot of small commits to fine-tune the script, unfortunately.

clauswilke

Overall looks good. Just a few remaining items while you're at it.

Don't worry about squashing the commits, GitHub does that automatically when merging.

clauswilke · 2020-12-22T00:45:03Z

.github/workflows/test.yml

+          - {os: ubuntu-20.04,   r: 'release', rust-version: 'stable', rspm: "https://packagemanager.rstudio.com/cran/__linux__/focal/latest"}
+          - {os: ubuntu-20.04,   r: 'release', rust-version: 'nightly', rspm: "https://packagemanager.rstudio.com/cran/__linux__/focal/latest"}
+          # R-devel requires LD_LIBRARY_PATH
+          - {os: ubuntu-20.04,   r: 'devel',   rust-version: 'nightly', rspm: "https://packagemanager.rstudio.com/cran/__linux__/focal/latest"}   


I think R devel should be tested with rust stable, so if something breaks we know whether it's R devel or rust nightly that is the problem. Also, could you expand the matrix so there is an R oldrel run for each OS/architecture? I'd like to have bindings for those versions as well (I realize this will break the test of precompiled bindings; don't worry about that or download the artifacts and add them, as you wish).

Ok, we now test three versions of R for each architecture. I also added R-release rust-nightly generating no bindings to test rust features.

Actually, you were wrong about tests -- you (or someone else) implemented a fallback to default bindings if the exact version is missing, so tests were successful (and a warning was logged). I downloaded new bindings, now we have fallback, 4.0 release, 4.1 devel, and 3.6 oldrel versions for each target.

clauswilke · 2020-12-22T00:47:44Z

.github/workflows/test.yml

        uses: actions/upload-artifact@main
        with:
-          name: Windows generated bindings
+          name: ${{ matrix.config.os }} ${{ matrix.config.rust-version }} generated bindings


The generated bindings need to have the R version in the name also.

Artifact names are now identical to job names

clauswilke · 2020-12-22T15:32:59Z

You're correct. I added the fallback if bindings are missing. Thanks!

* Split Windows jobs in two * Setting up GH actions environment * Fixing indentation * Fixing indentation #2 * Testing environment * Fixe incorrect rustup command * Checking CARGO_BUILD_FLAGS * Running test build * Probing environment * Testing new build * Fixing GITHUB_ENV/GITHUB_PATH * Enabling tests * Enabling macOS * Enabling tests of precomputed bindings * Exporting bindings on success * Checking Windows paths * Trying Win-i686 build * Temporary disable ubuntu::r-devel * Comment * Upload bindings if both build and test succeed * Added RSPM * Disabled ubuntu::r-devel; generate bindings only for stable rust * Removed old code * Fixed rust version comparison string * Testing 'if:' condition * Fixed config * Testing ubuntu::r-devel * Bypassing R alias * Debugging environment * Cleaning * Removed unused R_HOME * Changed generated bindings name * Adding Windows/MacOS workflows * New bindings for Windows/MacOS * Adding Ubuntu jobs * Bindings for Linux

Split Windows jobs in two

94c6574

Ilia-Kosenkov added 23 commits December 21, 2020 19:27

Setting up GH actions environment

7c0753a

Fixing indentation

36e48a0

Fixing indentation #2

9464e1e

Testing environment

59ca855

Fixe incorrect rustup command

efbac57

Checking CARGO_BUILD_FLAGS

677f57f

Running test build

3c60921

Probing environment

bad5bd1

Testing new build

fb44abb

Fixing GITHUB_ENV/GITHUB_PATH

d0d3a22

Enabling tests

d697e7c

Enabling macOS

96de673

Enabling tests of precomputed bindings

7ce611f

Exporting bindings on success

825bc96

Checking Windows paths

414a62a

Trying Win-i686 build

a7bdc2a

Temporary disable ubuntu::r-devel

e06db21

Comment

30200c4

Upload bindings if both build and test succeed

384bfec

Added RSPM

4d508d2

Disabled ubuntu::r-devel; generate bindings only for stable rust

853c0f5

Removed old code

d7085fa

Fixed rust version comparison string

d583aeb

clauswilke reviewed Dec 21, 2020

View reviewed changes

Ilia-Kosenkov added 7 commits December 22, 2020 00:39

Testing 'if:' condition

b48ba58

Fixed config

69f38b5

Testing ubuntu::r-devel

b9a436c

Bypassing R alias

3b28751

Debugging environment

28da00b

Cleaning

ca67e18

Removed unused R_HOME

26c6ba7

clauswilke reviewed Dec 22, 2020

View reviewed changes

Ilia-Kosenkov added 5 commits December 22, 2020 16:42

Changed generated bindings name

283b268

Adding Windows/MacOS workflows

2e4b0ea

New bindings for Windows/MacOS

9265886

Adding Ubuntu jobs

e1741f2

Bindings for Linux

45d5b58

clauswilke merged commit 1251100 into extendr:master Dec 22, 2020

Ilia-Kosenkov deleted the gha-win-i686-build branch December 22, 2020 18:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Github Actions for Windows #34

Update Github Actions for Windows #34

Ilia-Kosenkov commented Dec 21, 2020

clauswilke commented Dec 21, 2020

clauswilke commented Dec 21, 2020

Ilia-Kosenkov commented Dec 21, 2020

Ilia-Kosenkov commented Dec 21, 2020

clauswilke commented Dec 21, 2020

clauswilke Dec 21, 2020

clauswilke Dec 21, 2020

clauswilke Dec 21, 2020

Ilia-Kosenkov commented Dec 21, 2020

Ilia-Kosenkov commented Dec 21, 2020 •

edited

Loading

clauswilke left a comment

clauswilke Dec 22, 2020

Ilia-Kosenkov Dec 22, 2020

clauswilke Dec 22, 2020

Ilia-Kosenkov Dec 22, 2020

clauswilke commented Dec 22, 2020

Update Github Actions for Windows #34

Update Github Actions for Windows #34

Conversation

Ilia-Kosenkov commented Dec 21, 2020

clauswilke commented Dec 21, 2020

clauswilke commented Dec 21, 2020

Ilia-Kosenkov commented Dec 21, 2020

Ilia-Kosenkov commented Dec 21, 2020

clauswilke commented Dec 21, 2020

clauswilke Dec 21, 2020

Choose a reason for hiding this comment

clauswilke Dec 21, 2020

Choose a reason for hiding this comment

clauswilke Dec 21, 2020

Choose a reason for hiding this comment

Ilia-Kosenkov commented Dec 21, 2020

Ilia-Kosenkov commented Dec 21, 2020 • edited Loading

clauswilke left a comment

Choose a reason for hiding this comment

clauswilke Dec 22, 2020

Choose a reason for hiding this comment

Ilia-Kosenkov Dec 22, 2020

Choose a reason for hiding this comment

clauswilke Dec 22, 2020

Choose a reason for hiding this comment

Ilia-Kosenkov Dec 22, 2020

Choose a reason for hiding this comment

clauswilke commented Dec 22, 2020

Ilia-Kosenkov commented Dec 21, 2020 •

edited

Loading