Skip to content

Conversation

@gnguy
Copy link
Contributor

@gnguy gnguy commented Nov 12, 2019

Addressing https://issues.apache.org/jira/browse/ARROW-6960 by adding lz4 and zstd, which was added in rtools via r-windows/rtools-packages#47

@github-actions
Copy link

Thanks for opening a pull request!

Could you open an issue for this pull request on JIRA?
https://issues.apache.org/jira/browse/ARROW

Then could you also rename pull request title in the following format?

ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}

See also:

@gnguy gnguy changed the title Add lz4 and zstd to R PKGBUILD ARROW-6960: [R] Add lz4 and zstd to R PKGBUILD Nov 12, 2019
@github-actions
Copy link

Copy link
Member

@nealrichardson nealrichardson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part seems good (the C++ part built fine on Appveyor; the failure seemed to be a flaky download: https://ci.appveyor.com/project/ApacheSoftwareFoundation/arrow/builds/28808032/job/e8t2eneqoavufqx5) but you'll also need to add these new libs to https://github.com/apache/arrow/blob/master/ci/windows-pkg-arrow-for-r.sh, otherwise the compression libraries won't be bundled.

@nealrichardson
Copy link
Member

There's a pretty big backup on https://ci.appveyor.com/project/ApacheSoftwareFoundation/arrow/, lots of builds fighting for too few workers, so feel free to post a link to a passing build on your fork, when you get there.

Also, you can verify that the compression support is working by downloading the r\arrow_0.15.1.9000.zip artifact (the binary R package) from the Appveyor job and installing it locally (see https://ci.appveyor.com/project/ApacheSoftwareFoundation/arrow/builds/28797790/job/6l2ewckvujk48vtb/artifacts for example artifacts).

You could also add a unit test that codec_is_available("lz4") or some other exercise of the compression that only runs on Appveyor.

Copy link
Member

@nealrichardson nealrichardson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! One small note about the test, but aside from that, looks good (pending CI)!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding this. A few points:

  • I think you could put this under test-compressed.R where there are other codec tests
  • Change the test name (in test_that() to something like "Compression codecs are included in the Windows build" because that's what we're testing, not that codec_is_available works
  • Wrap the assertions (or even the whole test_that()) in if (identical(Sys.getenv("APPVEYOR"), "True")) { ... } (i.e. the inverse of what testthat::skip_on_appveyor() does) because we're testing that that Windows build is including the right stuff. It's not so much a test of the code as of the build system.

@nealrichardson
Copy link
Member

@gnguy I'm heading out on vacation and didn't want to leave you hanging on this so I just did the test change I suggested. Will merge when CI is green. Thanks again!

@nealrichardson
Copy link
Member

@ursabot crossbow submit macos-r-autobrew

@ursabot
Copy link

ursabot commented Nov 18, 2019

AMD64 Conda Crossbow Submit (#77866) builder has been succeeded.

Revision: c4d7e96

Submitted crossbow builds: ursa-labs/crossbow @ ursabot-333

Task Status
macos-r-autobrew TravisCI

@nealrichardson
Copy link
Member

nealrichardson commented Nov 18, 2019

Watching appveyor on https://ci.appveyor.com/project/nealrichardson/arrow/builds/28950140, which will finish hours before the ASF job will start.

@nealrichardson
Copy link
Member

Appveyor passed here: https://ci.appveyor.com/project/nealrichardson/arrow/builds/28950516

Everything else was green. Merging now.

Thanks @gnguy !

@gnguy
Copy link
Contributor Author

gnguy commented Nov 19, 2019

Awesome, thanks so much @nealrichardson!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants