Update GitHub Actions workflows, and make necessary tweaks to support them #279

DavisVaughan · 2022-08-22T13:14:47Z

This PR was originally intended to update the GitHub Actions workflow files in cpp11, but doing so proved to be quite challenging since they haven't been updated in awhile.

Note that there are more files changed here than just the workflow files, because we needed to tweak some of the actual cpp11 tests to accommodate some other problems. I will point them out with inline comments describing them in detail.

You should also be able to look at the individual commits to track the changes, I have tried to make them meaningful.

.github/workflows/R-CMD-check.yaml

Closes r-lib#280

Otherwise this will incorrectly return `TRUE` because it compiles a C program rather than a C++ one to check if color support is available, which adds the `-fdiagnostics-color=always` flag, but g++ 4.8 doesn't actually support that, so the compilation of the C++ code in the vignettes would fail

DavisVaughan · 2022-08-24T14:04:26Z

.github/workflows/R-CMD-check.yaml

          - {os: windows-latest, r: 'release'}
-          - {os: windows-latest, r: '3.6', rspm: "https://packagemanager.rstudio.com/cran/latest"}
-          - {os: ubuntu-18.04,   r: 'devel', rspm: "https://packagemanager.rstudio.com/cran/__linux__/bionic/latest", http-user-agent: "R/4.1.0 (ubuntu-18.04) R (4.1.0 x86_64-pc-linux-gnu x86_64 linux-gnu) on GitHub Actions" }


GitHub Actions is no longer supporting Ubuntu 18.04, so we switched to 20.04.

Note that r-lib/actions actually just switched to ubuntu-latest as the default, but I think for cpp11 it is worth trying to continue to test on "older" Ubuntu versions like 20.04 r-lib/actions#612

DavisVaughan · 2022-08-24T14:08:30Z

.github/workflows/R-CMD-check.yaml

+        # We check on this old compiler specifically to support CentOS 7,
+        # which uses this gcc version. RStudio products support CentOS 7 through
+        # June 2024.
+        # https://github.com/r-lib/cpp11/pull/78
+        # https://www.rstudio.com/about/platform-support/
+        # Ubuntu 20.04 technically dropped support for gcc 4.8, so we have to
+        # add old archives back in manually to install it
+        # https://github.com/r-lib/cpp11/pull/279
        if: matrix.config.custom == 'gcc 4.8'
        run: |
+          echo "deb http://dk.archive.ubuntu.com/ubuntu/ xenial main" | sudo tee -a /etc/apt/sources.list
+          echo "deb http://dk.archive.ubuntu.com/ubuntu/ xenial universe" | sudo tee -a /etc/apt/sources.list
+          sudo apt update


I have retained support for testing gcc 4.8, but it wasn't easy. The move to Ubuntu 20.04 means that sudo apt-get install -y g++-4.8 doesn't work out of the box anymore.

The way to fix this is to add older archives into the sources.list manually. This was proposed on a number of stack overflow questions, like these
https://stackoverflow.com/questions/68805002/i-need-to-install-gcc4-9-on-ubuntu-20-04-matlab-mex
https://askubuntu.com/questions/1036108/install-gcc-4-9-at-ubuntu-18-04

Corresponds to this commit 6a2994a

Calling out the fact that RStudio are doing this to support CentOS7, I wonder if it would make sense to move to using docker for CentOS 7 explicitly, in order to better replicate the environment you are trying to support, IIRC you would have to use Docker for everything then, so maybe not worth it, or you could run this in a separate workflow...

I opened #281 to consider this

Arrow uses a docker image in this way, so we could probably do the same

DavisVaughan · 2022-08-24T14:16:04Z

.github/workflows/R-CMD-check.yaml

          sudo apt-get install -y g++-4.8
          mkdir ~/.R/
-          echo $'CXX1X=g++-4.8\nCXX11=g++-4.8' >> ~/.R/Makevars
+          echo $'CXX1X=g++-4.8\nCXX11=g++-4.8\nCC=gcc-4.8' >> ~/.R/Makevars


The addition of CC is fairly tricky. If you don't add this, the FAQ vignette fails to build with errors about -fdiagnostics-color=always being a flag that isn't supported. Note that the FAQ vignette has {cpp11} knitr chunks that it compiles, and while it was compiling those chunks it was indeed setting the -fdiagnostics-color=always flag.

Some research turned up that this flag was only supported in gcc >=4.9, so gcc is right that it shouldn't be there.

It turns out this flag was being added by pkgbuild during pkgbuild::build().
https://github.com/r-lib/pkgbuild/blob/80762db02d9273997ed6234f654633359269cc9e/R/compiler-flags.R#L51-L58
https://github.com/r-lib/pkgbuild/blob/80762db02d9273997ed6234f654633359269cc9e/R/build.R#L54

It gets added if it looks like the compiler supports colored diagnostics, i.e. the has_compiler_colored_diagnostics() check there. This helper works by attempting to compile a simple C program with -fdiagnostics-color=always set in the Makevars https://github.com/r-lib/pkgbuild/blob/80762db02d9273997ed6234f654633359269cc9e/R/compiler-flags.R#L76. If the compilation fails, we assume there is no color support. The problem is that it attempts this on a C program which uses a compiler that does support color, but the C++ compiler we set through CXX11 does not.

So my fix for this is to just force the C compiler to also be gcc 4.8 so this helper returns FALSE.

Reported to pkgbuild here r-lib/pkgbuild#141

Corresponds to this commit f59c833

yeah, that sounds right, I wonder if pkgbuild should test both the CC and CXX points.

Alternatively perhaps it is better practice to use the same toolchain for C and C++ anyway (even though there is no C code being compiled here)

I wonder if pkgbuild should test both the CC and CXX points.

Gabor and I are talking about exactly that in r-lib/pkgbuild#141

DavisVaughan · 2022-08-24T14:47:32Z

.github/workflows/pkgdown.yaml

          needs: website

-      - name: Deploy package
+      - name: Build site
        env:
          CPP11_EVAL: true


In particular, made sure to retain this!

DavisVaughan · 2022-08-24T14:47:53Z

.github/workflows/pkgdown.yaml

-          extra-packages: local::. local::cpp11test pkgdown
+          extra-packages: any::pkgdown, local::., local::cpp11test


Retained installation of cpp11test here

DavisVaughan · 2022-08-24T14:49:26Z

.github/workflows/test-coverage.yaml

-          pak::local_install(dependencies = FALSE)
+          options(warn = 2)
          pak::local_install_dev_deps("cpp11test", dependencies = TRUE)
-          install.packages("cpp11test", repos = NULL, INSTALL_opts = "--install-tests")
-          pak::local_install(dependencies = TRUE)
-          pak::pkg_install("covr")
+          install.packages("cpp11test", repos = NULL, INSTALL_opts = "--install-tests", type = "source")


Mainly just syncing this to what is in R-CMD-check.yaml

DavisVaughan · 2022-08-24T14:51:33Z

DESCRIPTION

+Remotes:
+    r-lib/testthat


There was a testthat issue that was causing some warnings when compiling cpp11test on Windows with the "latest" version of R. Since we use options(warn = 2) before compiling it was causing the build to fail r-lib/testthat#1672

Corresponds to this commit 7a479d8

wonder if we should pin this to a commit SHA rather than having it float, and remove the pin after the changes are on CRAN.

I worry that having it float with the HEAD would cause builds to fail due to unrelated testthat development

DavisVaughan · 2022-08-24T14:55:11Z

cpp11test/src/test-doubles.cpp

-    SEXP x = PROTECT(Rf_coerceVector(R_compact_intrange(1, 5), REALSXP));
+    // ALTREP compact-seq
+    auto seq = cpp11::package("base")[":"];
+    cpp11::sexp range = seq(cpp11::as_sexp(1), cpp11::as_sexp(5));
+
+    SEXP x = PROTECT(Rf_coerceVector(range, REALSXP));
+    expect_true(ALTREP(x));


It turns out that R_compact_intrange() wasn't supposed to be exposed to us, and R core has moved it to Defn.h in R 4.2.0 (or maybe 4.2.1, not sure)
wch/r-source@86b4a06

I've replaced it with a call to 1:5, which does make an ALTREP object that we can use here, and seemed like a reasonable way to make one for now (with the alternative being that we create a full altrep dummy class in cpp11test to test with)

Corresponds to this commit 29cc833

Makes sense, wonder if it is more readable to use seq rather than :, though the former is a S3 generic, so not quite the same thing, I don't think it will ever matter in practice.

DavisVaughan · 2022-08-24T14:59:06Z

cpp11test/src/test-protect.cpp

+// Avoid unused variable warnings regarding `cpp11::preserved`
+// clang-format off
+#ifdef __clang__
+# pragma clang diagnostic push
+# pragma clang diagnostic ignored "-Wunused-variable"
+#endif
+
+#ifdef __GNUC__
+# pragma GCC diagnostic push
+# pragma GCC diagnostic ignored "-Wunused-variable"
+#endif
+// clang-format on


Another thing that showed up on the Windows build with the "latest" version of R while compiling cpp11test was a warning about cpp11::preserved being an unused variable in this test-protect.cpp file.

I guess that makes sense, because it isn't used anywhere in this test file but is created as a static struct in the header file so each compilation unit gets a copy of it and is "expected" to use it. Not sure why we don't get this warning on other OSes, but I just copied over the same pragma from protect.cpp that is also used to ignore some unused variable warnings

cpp11/inst/include/cpp11/protect.hpp

Line 301 in cb1a688

static struct {

cpp11/cpp11test/src/protect.cpp

Line 37 in cb1a688

#ifdef __clang__

Corresponds to this commit 0fbdcb1

DavisVaughan · 2022-08-24T15:19:33Z

@jimhester would you mind reviewing this PR for me? There are a number of things here where I want to be sure that I am correctly preserving what was already there, and I think having a review from you would be very helpful for that.

I would have broken it up into smaller individual PRs if I could, but it seemed more useful to have 1 PR with well defined commits that results in all builds passing again.

jimhester

Looks good to me, sorry about all the finickiness, cpp11 is kind of in a weird place with its interaction with CI

Bump Ubuntu version from 18.04 to 20.04

5ceee8c

DavisVaughan marked this pull request as ready for review August 22, 2022 13:14

Fully update GitHub Actions workflows

a4fb06c

DavisVaughan changed the title ~~Bump Ubuntu version from 18.04 to 20.04~~ Fully update GitHub Actions workflow files Aug 22, 2022

IndrajeetPatil reviewed Aug 22, 2022

View reviewed changes

.github/workflows/R-CMD-check.yaml Show resolved Hide resolved

DavisVaughan force-pushed the feature/bump-gha branch 2 times, most recently from deeaa25 to 7ac5c77 Compare August 23, 2022 00:52

Avoid R_compact_intrange() in favor of :

29cc833

Closes r-lib#280

DavisVaughan force-pushed the feature/bump-gha branch from 7ac5c77 to 29cc833 Compare August 23, 2022 01:03

Avoid unused variable warning on Windows about cpp11::preserved

0fbdcb1

DavisVaughan force-pushed the feature/bump-gha branch 2 times, most recently from ea01b66 to 0fbdcb1 Compare August 23, 2022 14:29

This comment was marked as outdated.

Sign in to view

Add a remote on r-lib/testthat#1673

d15902a

DavisVaughan force-pushed the feature/bump-gha branch from 00d0a3f to d15902a Compare August 23, 2022 22:22

DavisVaughan added 3 commits August 23, 2022 18:23

Add old archives to be able to install gcc 4.8

6a2994a

Depend only on dev testthat now that the PR has been merged

7a479d8

DavisVaughan force-pushed the feature/bump-gha branch from 204ef52 to f59c833 Compare August 24, 2022 02:07

DavisVaughan changed the title ~~Fully update GitHub Actions workflow files~~ Update GitHub Actions workflows, and make necessary tweaks to support them Aug 24, 2022

DavisVaughan commented Aug 24, 2022

View reviewed changes

NEWS bullet

be8e39c

DavisVaughan requested a review from jimhester August 24, 2022 15:19

DavisVaughan mentioned this pull request Aug 24, 2022

-fdiagnostics-color=always can sometimes be added when it isn't supported r-lib/pkgbuild#141

Closed

jimhester approved these changes Aug 24, 2022

View reviewed changes

DavisVaughan mentioned this pull request Aug 25, 2022

Move the Ubuntu gcc 4.8 workflow to it's own workflow file #281

Closed

DavisVaughan added 2 commits August 25, 2022 09:19

Pin to a specific testthat SHA

3572e7c

Use seq() rather than : to generate the ALTREP compact-seq vector

5ad4dba

DavisVaughan merged commit 3e23b30 into r-lib:main Aug 25, 2022

DavisVaughan deleted the feature/bump-gha branch August 25, 2022 14:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update GitHub Actions workflows, and make necessary tweaks to support them #279

Update GitHub Actions workflows, and make necessary tweaks to support them #279

DavisVaughan commented Aug 22, 2022 •

edited

Loading

This comment was marked as outdated.

DavisVaughan Aug 24, 2022

DavisVaughan Aug 24, 2022

jimhester Aug 24, 2022

DavisVaughan Aug 25, 2022 •

edited

Loading

DavisVaughan Aug 24, 2022 •

edited

Loading

jimhester Aug 24, 2022

DavisVaughan Aug 24, 2022

DavisVaughan Aug 24, 2022

DavisVaughan Aug 24, 2022

DavisVaughan Aug 24, 2022

DavisVaughan Aug 24, 2022

jimhester Aug 24, 2022

DavisVaughan Aug 24, 2022

jimhester Aug 24, 2022

DavisVaughan Aug 24, 2022

DavisVaughan commented Aug 24, 2022

jimhester left a comment

		extra-packages: local::. local::cpp11test pkgdown
		extra-packages: any::pkgdown, local::., local::cpp11test

		Remotes:
		r-lib/testthat

Update GitHub Actions workflows, and make necessary tweaks to support them #279

Update GitHub Actions workflows, and make necessary tweaks to support them #279

Conversation

DavisVaughan commented Aug 22, 2022 • edited Loading

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavisVaughan Aug 25, 2022 • edited Loading

Choose a reason for hiding this comment

DavisVaughan Aug 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavisVaughan commented Aug 24, 2022

jimhester left a comment

Choose a reason for hiding this comment

DavisVaughan commented Aug 22, 2022 •

edited

Loading

DavisVaughan Aug 25, 2022 •

edited

Loading

DavisVaughan Aug 24, 2022 •

edited

Loading