ARROW-3759: [R][CI] Build and test (no libarrow) on Windows in Appveyor #4538

nealrichardson · 2019-06-12T23:09:05Z

This is very much a work-in-progress for running the R package tests on Appveyor. It's an attempt to bring in the "standard" R Appveyor build tooling (https://github.com/krlmlr/r-appveyor, Apache 2.0 licensed) into Arrow's configuration.

I currently have the C++ library building and installing, and R and all R package dependencies installing, and R checks beginning, but they fail. This is because https://github.com/apache/arrow/blob/master/r/tools/winlibs.R is configured only to build with a released [sic] version of the arrow C++ binaries pre-built and hosted on rwinlib. Two immediate to-dos are:

get the package to install and check without libarrow at all (as ARROW-5488: [R] Workaround when C++ lib not available #4471 now allows)
get the package to find the libarrow we built locally from source

After that, I can reverse-engineer what appveyor does and document it for any Windows developers (https://issues.apache.org/jira/browse/ARROW-3758).

There's obviously some mess I've made of appveyor.yml that I'll clean up before I'm done, and I'll do my best to document and refactor some of the non-obvious features of the setup that I've discovered (e.g. that ci/appveyor-cpp-build-mingw.bat also installs GLib and Ruby and runs their test suites too) while I'm at it so that the next person might be able to pick things up faster.

cc @romainfrancois @javierluraschi

romainfrancois · 2019-06-13T12:05:32Z

#4471 is driven by the configure script, which is ignored on windows

nealrichardson · 2019-06-13T21:53:42Z

This is ready for review now. This adds the foundations for building the R package on Windows and on Appveyor, and it adds an Appveyor job to check the R package without the C++ library. This includes some refactoring of the configure script (adding configure.win) and the tools/winlibs.R script that will be necessary for the package to work even on the happy path where there's a build binary on rwinlibs, now that there's the new ARROW_R_WITH_ARROW flag. I.e. without this change, if we did a repeat of the Windows build process described here, we wouldn't get a functioning package.

Getting the R package to use the C++ library built from source will require integrating the scripts and workflow described there; IIUC we can't just use the C++ library that our current appveyor scripts build and have to compile with the Rtools toolchain. Given that that will be a bit of work, I'd like to separate that effort and get this piece merged now.

… release script

wesm

Overall seems OK, the title of the PR is a bit misleading since the R unit tests aren't actually being run in Appveyor yet

wesm · 2019-06-17T15:02:37Z

dev/release/00-prepare-test.rb

-                       ["-VERSION = #{@release_version}",
-                        "+VERSION = #{@release_version}.9000"],
-                     ],
-                   },


What are these changes about?

The 00-prepare.sh does not modify Makevars.win anymore, the generation of this file is now deferred at installation (@nealrichardson please confirm) via the added configure.win.

This patch makes Makevars.win be generated dynamically, so there's no longer a file that needs to be updated in the release scripts.

nealrichardson · 2019-06-17T18:30:22Z

Title revised--that was the ticket title and original scope, but as I got into it, it made sense to stop short of the grand goal in this iteration. FTR it is running the R package checks, including tests; all tests that require libarrow are skipped, but we are testing that conditional skipping behavior and that the package successfully builds if libarrow isn't found. This fallback behavior is not working on Windows on master, and this PR fixes that. It is important that this behavior work, if for no other reason than to prevent a flaky build failure (on CRAN or elsewhere).

appveyor.yml

fsaintjacques · 2019-06-17T18:12:57Z

r/configure.win

+echo "*** Writing Makevars.win"
+sed -e "s|@cflags@|$PKG_CFLAGS|" -e "s|@libs@|$PKG_LIBS|" src/Makevars.in > src/Makevars.win
+# Success
+exit 0


Small bash nit, exit 0 does not need to be explicited since it's implicit that a script returns 0.

This is copypasta from r/configure 🤷‍♂

fsaintjacques · 2019-06-17T18:25:46Z

dev/release/00-prepare-test.rb

-                       ["-VERSION = #{@release_version}",
-                        "+VERSION = #{@release_version}.9000"],
-                     ],
-                   },


The 00-prepare.sh does not modify Makevars.win anymore, the generation of this file is now deferred at installation (@nealrichardson please confirm) via the added configure.win.

fsaintjacques · 2019-06-17T18:30:22Z

r/configure.win

+VERSION=$(grep ^Version DESCRIPTION | sed s/Version:\ //)
+"${R_HOME}/bin${R_ARCH_BIN}/Rscript.exe" "tools/winlibs.R" $VERSION
+
+if [ $? -ne 0 ]; then


Is this expected? And should you exit immediately if this failure is encountered.

tools/winlibs.R will exit nonzero if https://github.com/apache/arrow/pull/4538/files/328bc376f9adb2717d1505f96d8d7452ee9d8212#diff-48db18073973d5ea65a7f0023f920145R22 fails to find and download a prebuilt binary. This triggers the fallback build-package-without-libarrow behavior (see #4471).

I can add some comments to explain.

Ok now I understand more clearly. I think this warrants a comment or clearer message, e.g.

"Failed to download and extract libarrow runtime and dependencies. The R arrow package will compile with shim methods. Please run arrow::install_arrow() to install required runtime libraries and recompile this package with proper support."

Or separate this message in half, one for users, and a comment for maintainers.

Just pushed some comments

appveyor.yml

fsaintjacques · 2019-06-17T18:46:20Z

r/src/Makevars.win

-# specific language governing permissions and limitations
-# under the License.
-
-VERSION = 0.13.0.9000


@kou did something go wrong, I'd expect the version to be 0.14.0.9000. This is the values Rust and Java uses for example. This should apply to DESCRIPTION.

0.13.0.9000 is "correct" (here at least) for the dev version after 0.13 before 0.14.

Ah, so the inverse of Java?

Apparently so? IDK, the version math makes more sense to me the way that this is done in the R community, 0.13.0 < 0.13.0.9000 < 0.14.0. I'm not familiar with how Java handles this.

0.13.0.9000 is correct.

See also: https://issues.apache.org/jira/browse/ARROW-4113?focusedCommentId=16728608&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16728608

Rust and Java use 0.14.0-SNAPSHOT. They don't use 0.14.0.9000.

We use X.Y.Z-SNAPSHOT for languages that support PRE release by tag ("SNAPSHOT").
R doesn't support tag. So we use ".9000". ".9000" is commonly used in R.

cf. http://r-pkgs.had.co.nz/release.html for the R convention.

fsaintjacques · 2019-06-18T12:48:17Z

+1

nealrichardson added 11 commits June 12, 2019 13:25

Throw this at appveyor and see how far off we are

af1852e

Debug with fewer jobs in the matrix

94c9a42

Put mine first and fast-finish

39236f3

nm we already fast finish

8c3e898

Find shell script up a level

f7327ed

Bootstrap within the package dir

d5e8f4e

let the debugging begin

1ba4839

No popd?

b6f6c8d

cd this way

18803c3

Try building first, see where that goes

15c0342

Try this name hack

fb19911

romainfrancois added the Component: R label Jun 13, 2019

nealrichardson added 3 commits June 13, 2019 12:59

Enable R package to install on windows even if rwinlib isn't found

19b60df

Some appveyor.yml cleanup

f4cf5c3

Cleanups

567f041

nealrichardson added 2 commits June 13, 2019 15:44

Makevars.win is dynamically generated now, no need to tweak it in the…

53c3a20

… release script

Merge remote-tracking branch 'upstream/master' into r-appveyor

328bc37

fsaintjacques self-requested a review June 14, 2019 17:19

wesm reviewed Jun 17, 2019

View reviewed changes

nealrichardson changed the title ~~WIP ARROW-3759: [R][CI] Build and test on Windows in Appveyor~~ ARROW-3759: [R][CI] Build and test (no libarrow) on Windows in Appveyor Jun 17, 2019

fsaintjacques reviewed Jun 17, 2019

View reviewed changes

appveyor.yml Outdated Show resolved Hide resolved

fsaintjacques reviewed Jun 17, 2019

View reviewed changes

nealrichardson added 2 commits June 17, 2019 12:25

Merge upstream/master

84eef1b

PR feedback

6d6a74e

fsaintjacques closed this in 83c211c Jun 18, 2019

nealrichardson deleted the r-appveyor branch June 18, 2019 16:46

asfimport mentioned this pull request Jun 18, 2019

[R][CI] Build and test on Windows in Appveyor #20077

Closed

ARROW-3759: [R][CI] Build and test (no libarrow) on Windows in Appveyor #4538

ARROW-3759: [R][CI] Build and test (no libarrow) on Windows in Appveyor #4538

Uh oh!

Conversation

nealrichardson commented Jun 12, 2019

Uh oh!

romainfrancois commented Jun 13, 2019

Uh oh!

nealrichardson commented Jun 13, 2019

Uh oh!

wesm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nealrichardson commented Jun 17, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fsaintjacques Jun 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fsaintjacques commented Jun 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

fsaintjacques Jun 17, 2019 •

edited

Loading