Skip to content

first attempt at fixing broken backfill build #1958

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

Merged
merged 2 commits into from
Apr 30, 2024
Merged

Conversation

dsweber2
Copy link
Contributor

@dsweber2 dsweber2 commented Apr 24, 2024

Description

The buildprocess in #1957 broke seemingly caused by an update to the R version. This should fix that.

Changelog

  • R CMD CHECK for this package only works on R <= 4.2, so pin Dockerfile to rocker/tidyverse:4.2
  • Update Dockerfile to use pak and rspm to install dependencies via binaries (removed system dependencies install command because pak takes care of those), should speed things up
  • Tested the Dockerfile build process locally and it built successfully
  • Simplified backfill-corr-ci.yml (setup-r-dependencies already caches, so no need for the caching action)

dsweber2 and others added 2 commits April 24, 2024 17:05
* pin to rocker/tidyverse:4.2
* update Dockerfile to use pak and rspm for package installation
* add workflow_dispatch button
* simplify caching (r-lib/setup-r-dependencies caches by default)
* switch to Ubuntu latest
* keep R on 4.2
if: github.event.pull_request.draft == false
strategy:
matrix:
r-version: [4.2.1]
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion (optional): maybe we want to keep the matrix way of providing r-version for future flexibility? In some other packages, we specify several different R versions to run tests on.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's just add that in when we need it, CI-wise it'll be about the same amount of work then.

id: get-date
run: |
echo "::set-output name=date::$(/bin/date -u "+%Y%m%d")"
- name: Cache R packages
Copy link
Contributor

Choose a reason for hiding this comment

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

praise:

setup-r-dependencies already caches, so no need for the caching action

Nice! Good simplification here and above.

use-public-rspm: true
- name: Install linux dependencies
run: |
sudo apt-get install \
Copy link
Contributor

Choose a reason for hiding this comment

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

question: we don't need this section because r-lib/actions/setup-r-dependencies@v2 installs using pak, which as you mentioned above also installs dependencies?

Copy link
Contributor

Choose a reason for hiding this comment

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

@dshemetov dshemetov merged commit 3fc3254 into main Apr 30, 2024
15 checks passed
@dshemetov dshemetov deleted the backfillCI_fix branch April 30, 2024 20:18
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