Skip to content

GH-41450: [R][CI] rhub/container follow ons #41451

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.

Sign up for GitHub

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

Merged
jonkeane merged 6 commits into from
May 12, 2024
Merged

GH-41450: [R][CI] rhub/container follow ons #41451

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1f695df
remove rhub/containers work around and test skip on devel that's fixe…
jonkeane Apr 29, 2024
ad7b233
Remove centos7 and devtoolset hacks for R
jonkeane Apr 29, 2024
3185879
move to rhub/asan
jonkeane Apr 30, 2024
9664a4c
s/RDsan/R/
jonkeane Apr 30, 2024
cb4ecea
Copy old rhub sanitizer options
jonkeane Apr 30, 2024
9843835
add sanitizer suppression for purrr
jonkeane May 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions .env
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,9 +86,6 @@ ARROW_R_DEV=TRUE
R_PRUNE_DEPS=FALSE
TZ=UTC

# Any non-empty string will install devtoolset-${DEVTOOLSET_VERSION}
DEVTOOLSET_VERSION=

# Used through docker-compose.yml and serves as the default version for the
# ci/scripts/install_vcpkg.sh script. Prefer to use short SHAs to keep the
# docker tags more readable.
Expand Down
3 changes: 1 addition & 2 deletions .github/workflows/r.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,12 +192,11 @@ jobs:
fail-fast: false
matrix:
config:
- { org: "rhub", image: "ubuntu-gcc12", tag: "latest", devtoolset: "" }
- { org: "rhub", image: "ubuntu-gcc12", tag: "latest" }
env:
R_ORG: ${{ matrix.config.org }}
R_IMAGE: ${{ matrix.config.image }}
R_TAG: ${{ matrix.config.tag }}
DEVTOOLSET_VERSION: ${{ matrix.config.devtoolset }}
steps:
- name: Checkout Arrow
uses: actions/checkout@3df4ab11eba7bda6032a0b82a6bb43b11571feac # v4.0.0
Expand Down
3 changes: 0 additions & 3 deletions ci/docker/linux-r.dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,6 @@ ENV R_BIN=${r_bin}
ARG r_dev=FALSE
ENV ARROW_R_DEV=${r_dev}

ARG devtoolset_version=
ENV DEVTOOLSET_VERSION=${devtoolset_version}

ARG r_prune_deps=FALSE
ENV R_PRUNE_DEPS=${r_prune_deps}

Expand Down
3 changes: 0 additions & 3 deletions ci/scripts/java_jni_manylinux_build.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,6 @@ echo "=== Clear output directories and leftovers ==="
rm -rf ${build_dir}

echo "=== Building Arrow C++ libraries ==="
devtoolset_version=$(rpm -qa "devtoolset-*-gcc" --queryformat %{VERSION} | \
grep -o "^[0-9]*")
devtoolset_include_cpp="/opt/rh/devtoolset-${devtoolset_version}/root/usr/include/c++/${devtoolset_version}"
Copy link
Member

@kou kou May 15, 2024

Choose a reason for hiding this comment

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

@jonkeane Is it expected change?

Copy link
Member Author

@jonkeane jonkeane May 15, 2024

Choose a reason for hiding this comment

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

Aaaah, I might have been overly aggressive in removing from this file.

I thought we only used the devtoolset_version on the R CentOS 7 builds and that other places we added that to were borrowing from those and no longer necessary. But if we need to add it back here that's totally fine. Sorry for not confirming that and the hassle.

Copy link
Member

@kou kou May 15, 2024

Choose a reason for hiding this comment

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

No problem. :-)
Thanks for confirming this.

: ${ARROW_ACERO:=ON}
export ARROW_ACERO
: ${ARROW_BUILD_TESTS:=ON}
Expand Down
20 changes: 0 additions & 20 deletions ci/scripts/r_docker_configure.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,26 +67,6 @@ sloppiness = include_file_ctime
hash_dir = false" >> ~/.ccache/ccache.conf
fi

# Special hacking to try to reproduce quirks on centos using non-default build
# tooling.
if [[ -n "$DEVTOOLSET_VERSION" ]]; then
$PACKAGE_MANAGER install -y centos-release-scl
$PACKAGE_MANAGER install -y "devtoolset-$DEVTOOLSET_VERSION"

# Enable devtoolset here so that `which gcc` finds the right compiler below
source /opt/rh/devtoolset-${DEVTOOLSET_VERSION}/enable

# Build images which require the devtoolset don't have CXX17 variables
# set as the system compiler doesn't support C++17
if [ ! "`{R_BIN} CMD config CXX17`" ]; then
mkdir -p ~/.R
echo "CC = $(which gcc) -fPIC" >> ~/.R/Makevars
echo "CXX17 = $(which g++) -fPIC" >> ~/.R/Makevars
echo "CXX17STD = -std=c++17" >> ~/.R/Makevars
echo "CXX17FLAGS = ${CXX11FLAGS}" >> ~/.R/Makevars
fi
fi

if [ -f "${ARROW_SOURCE_HOME}/ci/scripts/r_install_system_dependencies.sh" ]; then
"${ARROW_SOURCE_HOME}/ci/scripts/r_install_system_dependencies.sh"
fi
Expand Down
2 changes: 2 additions & 0 deletions ci/scripts/r_sanitize.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ unset ARROW_R_DEV
export ARROW_R_VERBOSE_TEST=TRUE

export UBSAN_OPTIONS="print_stacktrace=1,suppressions=/arrow/r/tools/ubsan.supp"
# From the old rhub image https://github.com/r-hub/rhub-linux-builders/blob/master/fedora-clang-devel-san/Dockerfile
export ASAN_OPTIONS="alloc_dealloc_mismatch=0:detect_leaks=0:detect_odr_violation=0"

# run tests
pushd tests
Expand Down
3 changes: 0 additions & 3 deletions ci/scripts/r_test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,6 @@ if [ "$ARROW_USE_PKG_CONFIG" != "false" ]; then
fi

export _R_CHECK_COMPILATION_FLAGS_KNOWN_="${_R_CHECK_COMPILATION_FLAGS_KNOWN_} ${ARROW_R_CXXFLAGS}"
# These should generally be picked up, but are slightly wrong in rhub's containers it appears
# https://github.com/r-hub/containers/pull/63
export _R_CHECK_COMPILATION_FLAGS_KNOWN_="${_R_CHECK_COMPILATION_FLAGS_KNOWN_} -Wno-parentheses -Werror=format-security -Wp,-D_FORTIFY_SOURCE=3"

if [ "$ARROW_R_DEV" = "TRUE" ]; then
# These are sometimes used in the Arrow C++ build and are not a problem
Expand Down
1 change: 0 additions & 1 deletion dev/tasks/r/azure.linux.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ jobs:
export R_ORG={{ r_org }}
export R_IMAGE={{ r_image }}
export R_TAG={{ r_tag }}
export DEVTOOLSET_VERSION={{ devtoolset_version|default("") }}
export R_CUSTOM_CCACHE={{ r_custom_ccache|default("false") }}
docker-compose pull --ignore-pull-failures r
docker-compose build r
Expand Down
7 changes: 3 additions & 4 deletions dev/tasks/r/github.packages.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,14 +299,14 @@ jobs:
# choosing a binary on this OS. If libarrow_binary is TRUE, we're on
# an OS that is not in the allowlist, so we have to opt-in to use the
# binary. Other env vars used in r_docker_configure.sh can be added
# here (like devtoolset) and wired up in the later steps.
# here and wired up in the later steps.
- {image: "rhub/ubuntu-clang", libarrow_binary: "TRUE"}
# fedora-clang-devel cannot use binaries bc of libc++ (uncomment to see the error)
# - {image: "rhub/fedora-clang-devel", libarrow_binary: "TRUE"}
- {image: "rhub/ubuntu-release"} # currently ubuntu-22.04
- {image: "rocker/r-ver:4.0.0"} # ubuntu-20.04
- {image: "rstudio/r-base:4.1-focal"} # ubuntu-20.04
- {image: "rstudio/r-base:4.2-centos7", devtoolset: "8"}
- {image: "rstudio/r-base:4.1-focal"}
- {image: "rstudio/r-base:4.2-jammy"}
- {image: "rstudio/r-base:4.3-noble"}
Comment on lines +308 to 310
Copy link
Member Author

@jonkeane jonkeane Apr 29, 2024

Choose a reason for hiding this comment

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

These probably shouldn't be all ubuntus. Any suggestions on other images we should use? centos8? RHEL? debian?

Copy link
Member

@nealrichardson nealrichardson May 10, 2024

Choose a reason for hiding this comment

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

I think rockylinux8 gets at the centos/RHEL line.

steps:
# Get the arrow checkout just for the docker config scripts
Expand All @@ -317,7 +317,6 @@ jobs:
- name: Install system requirements
env:
ARROW_R_DEV: "TRUE" # To install curl/openssl in r_docker_configure.sh
DEVTOOLSET_VERSION: {{ '${{ matrix.config.devtoolset }}' }}
shell: bash
run: |
# Make sure R is on the path for the R-hub devel versions (where RPREFIX is set in its dockerfile)
Expand Down
13 changes: 2 additions & 11 deletions dev/tasks/tasks.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1411,15 +1411,6 @@ tasks:
GCC_VERSION: 12
image: ubuntu-r-only-r

test-r-rstudio-r-base-4.2-centos7-devtoolset-8:
ci: azure
template: r/azure.linux.yml
params:
r_org: rstudio
r_image: r-base
r_tag: 4.2-centos7
devtoolset_version: 8

test-r-minimal-build:
ci: azure
template: r/azure.linux.yml
Expand All @@ -1437,13 +1428,13 @@ tasks:
R_PRUNE_DEPS: TRUE
image: ubuntu-r-sanitizer

test-fedora-r-clang-sanitizer:
test-r-clang-sanitizer:
ci: github
template: docker-tests/github.linux.yml
params:
env:
R_PRUNE_DEPS: TRUE
image: fedora-r-clang-sanitizer
image: r-clang-sanitizer

{% for go_version, staticcheck in [("1.21", "v0.4.7"), ("1.22", "latest")] %}
test-debian-12-go-{{ go_version }}:
Expand Down
16 changes: 6 additions & 10 deletions docker-compose.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ x-hierarchy:
- ubuntu-r-valgrind
- ubuntu-swift
- ubuntu-verify-rc
- fedora-r-clang-sanitizer
- r-clang-sanitizer
- r
- r-revdepcheck
# helper services
Expand Down Expand Up @@ -1470,7 +1470,6 @@ services:
args:
base: ${R_ORG}/${R_IMAGE}:${R_TAG}
r_dev: ${ARROW_R_DEV}
devtoolset_version: ${DEVTOOLSET_VERSION}
tz: ${TZ}
r_prune_deps: ${R_PRUNE_DEPS}
r_custom_ccache: ${R_CUSTOM_CCACHE}
Expand All @@ -1482,7 +1481,6 @@ services:
ARROW_R_DEV: ${ARROW_R_DEV}
# To test for CRAN release, delete ^^ these two env vars so we download the Apache release
ARROW_USE_PKG_CONFIG: "false"
devtoolset_version: ${DEVTOOLSET_VERSION}
volumes:
- .:/arrow:delegated
command: >
Expand Down Expand Up @@ -1515,19 +1513,17 @@ services:
/bin/bash -c "
/arrow/ci/scripts/r_sanitize.sh /arrow"

fedora-r-clang-sanitizer:
image: ${REPO}:r-rhub-fedora-clang-devel-latest
r-clang-sanitizer:
image: ${REPO}:r-rhub-clang-devel-latest
build:
context: .
dockerfile: ci/docker/linux-r.dockerfile
cache_from:
- ${REPO}:r-rhub-fedora-clang-devel-latest
- ${REPO}:r-rhub-clang-devel-latest
args:
# TODO: change this to rhub/clang-asan
base: rhub/fedora-clang-devel-san
base: rhub/clang-asan
r_dev: ${ARROW_R_DEV}
devtoolset_version: ${DEVTOOLSET_VERSION}
r_bin: RDsan
r_bin: R
tz: ${TZ}
r_prune_deps: ${R_PRUNE_DEPS}
shm_size: *shm-size
Expand Down
4 changes: 0 additions & 4 deletions r/tools/test-nixlibs.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,10 +158,6 @@ test_that("check_allowlist", {
})

test_that("find_latest_nightly()", {
skip_if(
getRversion() > "4.4.0",
"long last version components (>8) fail to max on r-devel"
)
tf <- tempfile()
tf_uri <- paste0("file://", tf)
on.exit(unlink(tf))
Expand Down
1 change: 1 addition & 0 deletions r/tools/ubsan.supp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,4 @@
# under the License.

vptr:include/c++/8/bits/shared_ptr_base.h
function:cleancall.c
33 changes: 0 additions & 33 deletions r/vignettes/install.Rmd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,35 +28,6 @@ For `gcc`, this generally means version 7 or newer. Most contemporary Linux
distributions have a new enough compiler; however, CentOS 7 is a notable
exception, as it ships with gcc 4.8.

If you are on CentOS 7, to build arrow you will need to install a newer `devtoolset`, and you'll need to update R's Makevars to define the `CXX17` variables. This script installs `devtoolset-8` and configures R to be able to use C++17:
Copy link
Member

@nealrichardson nealrichardson May 10, 2024

Choose a reason for hiding this comment

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

The note in the paragraph above also mentions CentOS 7.

Maybe we should keep this discussion and say something like, we don't officially support CentOS 7 anymore, but if you want to try building on it, you'll need to:

Copy link
Member Author

@jonkeane jonkeane May 10, 2024

Choose a reason for hiding this comment

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

nods ok yeah, I also like that it leaves a bit of a more obvious breadcrumb trail for folks debugging. I know it's in the git history, but I don't think folks would know to look here in that history necessarily to find out "what do I do with non-standard compiler setups?"


```
#!/usr/bin/env bash

yum install -y centos-release-scl
yum install -y devtoolset-8
# Optional: also install cloud storage dependencies, as described below
yum install -y libcurl-devel openssl-devel

source /opt/rh/devtoolset-8/enable

if [ ! `R CMD config CXX17` ]; then
mkdir -p ~/.R
echo "CC = $(which gcc) -fPIC" >> ~/.R/Makevars
echo "CXX17 = $(which g++) -fPIC" >> ~/.R/Makevars
echo "CXX17STD = -std=c++17" >> ~/.R/Makevars
echo "CXX17FLAGS = ${CXX11FLAGS}" >> ~/.R/Makevars
fi
```

Note that the C++17 compiler is only required at *build* time. You don't need
to enable the devtoolset every time you load the package. What's more, if you
install a binary package from RStudio Package Manager (see method 1a below), you
do not need to set up any of this. Likewise, if you `R CMD INSTALL --build`
arrow on a CentOS machine with the newer compilers, you can take the binary
package it produces and install it on any other CentOS machine without those
compilers.

### Libraries

Optional support for reading from cloud storage--AWS S3 and
Expand Down Expand Up @@ -517,10 +488,6 @@ The install script should work everywhere, so if libarrow fails to compile,
please [report an issue](https://issues.apache.org/jira/projects/ARROW/issues)
so that we can improve the script.

### Known installation issues
Copy link
Member

@nealrichardson nealrichardson May 10, 2024

Choose a reason for hiding this comment

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

Similarly, maybe this should stick around in some form since we direct people here for troubleshooting.


* On CentOS, building the package requires a more modern `devtoolset` than the default system compilers. See "System dependencies" above.

## Contributing

We are constantly working to make the installation process as painless as
Expand Down