Skip to content

ARROW-7060: [R] Post-0.15.1 cleanup#5773

Closed
nealrichardson wants to merge 9 commits intoapache:masterfrom
nealrichardson:post-0.15.1
Closed

ARROW-7060: [R] Post-0.15.1 cleanup#5773
nealrichardson wants to merge 9 commits intoapache:masterfrom
nealrichardson:post-0.15.1

Conversation

@nealrichardson
Copy link
Member

The most substantive change here is revisions to r/configure based on a recently added CRAN check for bashisms. We could have caught that change earlier if we were running R tests on Travis with the latest development version of R, so this patch fixes that as well (even though we may not be using Travis for much longer).

There is also some backfilling of r/NEWS.md since I was already in the file.

Leaving this as a draft PR until the CRAN submission is accepted, in case we need to revise further.

@github-actions
Copy link

github-actions bot commented Nov 4, 2019

@nealrichardson nealrichardson marked this pull request as ready for review November 4, 2019 23:38
Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

LGTM, but I'm not very knowledgeable here. @kou do you want to take a look?

# -------------------------------------------------------------------------
- name: R (with and without libarrow)
language: r
r: devel
Copy link
Member

Choose a reason for hiding this comment

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

Does it use a development version of R or does it enable development-specific options (such as debugging perhaps?)?

Copy link
Member Author

Choose a reason for hiding this comment

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

The former. It installs a nightly build of R. It's important to test with R-devel in CI because CRAN checks package submissions against it, and the main reason for this patch is that there was an additional check in R-devel that arrow failed. We would have seen and fixed it three weeks ago if we'd been testing against R-devel.

Copy link
Member

Choose a reason for hiding this comment

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

I see. But how do we check for regressions with stable releases of R (I assume that's important)?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is important, and we should do both, but if I only get to run one job, it should be the development version. You're much more likely to have something pass on the stable release and fail on devel than vice versa.

Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

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

+1

if [ "$FORCE_AUTOBREW" != "TRUE" ] && [ "`command -v brew`" ] && [ "`brew ls --versions ${PKG_BREW_NAME}`" != "" ]; then
echo "Using Homebrew ${PKG_BREW_NAME}"
BREWDIR=$(brew --prefix)
BREWDIR=`brew --prefix`
Copy link
Member

Choose a reason for hiding this comment

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

Oh, we couldn't use $(...) on Solaris...

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, turns out we could not. This worked, though.

@nealrichardson nealrichardson deleted the post-0.15.1 branch November 6, 2019 05:17
homepage "https://arrow.apache.org/"
url "https://www.apache.org/dyn/closer.cgi?path=arrow/arrow-0.15.0.9000/apache-arrow-0.15.0.9000.tar.gz"
url "https://www.apache.org/dyn/closer.cgi?path=arrow/arrow-0.15.1.9000/apache-arrow-0.15.1.9000.tar.gz"
sha256 "9948ddb6d4798b51552d0dca3252dd6e3a7d0f9702714fc6f5a1b59397ce1d28"
Copy link
Member

Choose a reason for hiding this comment

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

This must be changed also?

Copy link
Member Author

@nealrichardson nealrichardson Nov 6, 2019

Choose a reason for hiding this comment

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

Yes: the release tests expect that this version match the R package version in r/DESCRIPTION. I had to bump that in this PR because, unlike a normal release, the patch release branch wasn't merged, so the R version wasn't bumped. Normally this is done automatically in the release.

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.

4 participants

Comments