Skip to content
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

ARROW-7665: [R] Build in parallel in linuxLibs.R #6271

Closed
wants to merge 3 commits into from

Conversation

pitrou
Copy link
Member

@pitrou pitrou commented Jan 23, 2020

Also fix the dependency relationship between Bison and Flex

Also fix the dependency relationship between Bison and Flex
ci/scripts/r_deps.sh Outdated Show resolved Hide resolved
@github-actions
Copy link

@@ -233,7 +240,7 @@ ensure_cmake <- function() {
}

# TODO: move ensure_flex/bison/m4 to cmake: https://issues.apache.org/jira/browse/ARROW-7501
ensure_flex <- function(m4 = ensure_m4()) {
ensure_flex <- function(m4 = ensure_m4(), bison = ensure_bison()) {
Copy link
Member

Choose a reason for hiding this comment

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

Why this change? bison seems to be an optional dependency of flex, and thrift seems to build and work just fine as it is. Did you have a build problem that this fixed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, yes, Flex fails building otherwise.

Copy link
Member

Choose a reason for hiding this comment

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

I of course would be happy to have this bison/flex code moved out of this R script and into cmake proper (https://issues.apache.org/jira/browse/ARROW-7501).

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm only interested in fixing this and speeding it up :)

Copy link
Member

Choose a reason for hiding this comment

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

How did you get Flex to fail to build otherwise? Maybe we should add that container to our build matrix. It's not failing for crossbow or GHA, and I never had it fail locally for me with those docker images (though I did see the message that flex was building without bison and chose not to care since thrift successfully built).

Copy link
Member Author

Choose a reason for hiding this comment

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

I ran the container locally, roughly like this:

$ export R_TAG=3.6-centos7
$ export R_IMAGE=r-base
$ export R_ORG=rstudio
$ docker-compose build r
$ docker-compose run --rm r

It's the same container as on Travis-CI, AFAICT.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah that's what's running on all commits and it's passing on master. It makes some noise about flex not having bison but it says not to worry: https://github.com/apache/arrow/runs/405798341#step:5:809

checking for bison... no
configure: no bison program found: only required for maintainers

I'm fine with the change if everything passes, but I suspect it's not necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I see... I saw the same error as here and thought that was the cause for the failing builds:
https://github.com/apache/arrow/runs/405798341#step:5:1028

I could try to remove those changes.

r/tools/linuxlibs.R Outdated Show resolved Hide resolved
r/tools/linuxlibs.R Outdated Show resolved Hide resolved
ci/scripts/r_deps.sh Outdated Show resolved Hide resolved
@nealrichardson
Copy link
Member

@ursabot crossbow submit -g r

@kszucs
Copy link
Member

kszucs commented Jan 23, 2020

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

Revision: 7de0f15

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

Task Status
macos-r-autobrew TravisCI
test-conda-r-3.6 CircleCI
test-r-rhub-debian-gcc-devel Azure
test-r-rhub-ubuntu-gcc-release Azure
test-r-rstudio-r-base-3.6-bionic Azure
test-r-rstudio-r-base-3.6-centos6 Azure
test-r-rstudio-r-base-3.6-opensuse15 Azure
test-r-rstudio-r-base-3.6-opensuse42 Azure
test-ubuntu-18.04-r-sanitizer CircleCI

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.

+1 Thanks! This cut the build time of the job that runs this script down from 23 to 12 minutes!

@pitrou pitrou deleted the ARROW-7665-faster-r-docker branch January 23, 2020 22:23
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.

None yet

3 participants