github-actions[bot]
commented
6 months ago Docker build status
| Commit time | Git sha | Image |
|---|---|---|
| 2024-04-30T16:03:11Z | 2992bca43b3de66fa7474dedd09095358f3fce9b | ghcr.io/rmi-pacta/workflow.data.preparation:pr-221 |
Closed AlexAxthelm closed 6 months ago
github-actions[bot]
commented
6 months ago | Commit time | Git sha | Image |
|---|---|---|
| 2024-04-30T16:03:11Z | 2992bca43b3de66fa7474dedd09095358f3fce9b | ghcr.io/rmi-pacta/workflow.data.preparation:pr-221 |
cjyetman
commented
6 months ago Do we need a Docker image created automatically on each PR? Also not a fan of the aggressive linting.
AlexAxthelm
commented
6 months ago Do we need a Docker image created automatically on each PR?
I think doing a build, but not running on each PR makes sense.
Also not a fan of the aggressive linting.
hadolint?
cjyetman
commented
6 months ago AlexAxthelm
commented
6 months ago I believe those changes are relevant to this PR, since they are cleaning up the Dockerfile so that hadolint and the check R system dependencies actions (which are introduced here) pass cleanly.
cjyetman
commented
6 months ago For example, building a linux/amd64 image is essentially a requirement, and removing that specification from the Dockerfile forces me to manually specify it every time I run this. So I'm very unlikely to approve something that both:
AlexAxthelm
commented
6 months ago I've restored the --platform=linux/amd64 in the FROM command, and told hadolint to exclude that check.
The rest of the changes to the Dockerfile are reasonable, and in line with best practices.
cjyetman
commented
6 months ago Where does cmake come from?
pak::pkg_sysreqs(pkg = "rmi-pacta/workflow.data.preparation", sysreqs_platform = "ubuntu-20.04")
#> ℹ Loading metadata database
#> ✔ Loading metadata database ... done
#>
#> ── Install scripts ───────────────────────────────────────────── Ubuntu 20.04 ──
#> apt-get -y update
#> apt-get -y install libicu-dev libxml2-dev git libcurl4-openssl-dev libssl-dev \
#> make libgit2-dev zlib1g-dev pandoc libfreetype6-dev libjpeg-dev libpng-dev \
#> libtiff-dev libfontconfig1-dev libfribidi-dev libharfbuzz-dev libnode-dev
#>
#> ── Packages and their system dependencies ──────────────────────────────────────
#> credentials – git
#> curl – libcurl4-openssl-dev, libssl-dev
#> fs – make
#> gert – libgit2-dev
#> gitcreds – git
#> httpuv – make, zlib1g-dev
#> knitr – pandoc
#> openssl – libssl-dev
#> pkgdown – pandoc
#> ragg – libfreetype6-dev, libjpeg-dev, libpng-dev, libtiff-dev
#> remotes – git
#> rmarkdown – pandoc
#> sass – make
#> stringi – libicu-dev
#> systemfonts – libfontconfig1-dev, libfreetype6-dev
#> textshaping – libfreetype6-dev, libfribidi-dev, libharfbuzz-dev
#> V8 – libnode-dev
#> xml2 – libxml2-dev
pak::pkg_sysreqs(pkg = "rmi-pacta/workflow.data.preparation", sysreqs_platform = "ubuntu-22.04")
#> ── Install scripts ───────────────────────────────────────────── Ubuntu 22.04 ──
#> apt-get -y update
#> apt-get -y install libicu-dev git libcurl4-openssl-dev libssl-dev make \
#> libgit2-dev zlib1g-dev pandoc libfreetype6-dev libjpeg-dev libpng-dev \
#> libtiff-dev libfontconfig1-dev libfribidi-dev libharfbuzz-dev libxml2-dev \
#> libnode-dev
#>
#> ── Packages and their system dependencies ──────────────────────────────────────
#> credentials – git
#> curl – libcurl4-openssl-dev, libssl-dev
#> fs – make
#> gert – libgit2-dev
#> gitcreds – git
#> httpuv – make, zlib1g-dev
#> knitr – pandoc
#> openssl – libssl-dev
#> pkgdown – pandoc
#> ragg – libfreetype6-dev, libjpeg-dev, libpng-dev, libtiff-dev
#> remotes – git
#> rmarkdown – pandoc
#> sass – make
#> stringi – libicu-dev
#> systemfonts – libfontconfig1-dev, libfreetype6-dev
#> textshaping – libfreetype6-dev, libfribidi-dev, libharfbuzz-dev
#> V8 – libnode-dev
#> xml2 – libxml2-devWhere does
cmakecome from?
cjyetman
commented
6 months ago why does it think this needs arrow?
cjyetman
commented
6 months ago I'm pretty sure pak::sysreqs_check_installed() (used here https://github.com/RMI-PACTA/actions/blob/ff62270b68b67c387bf388372ad667b21e6b8fca/.github/workflows/docker-check-R-sysdeps.yml#L20C20-L20C50) is looking for all development dependencies including those in Suggests, which is picking up arrow from DBI.
https://github.com/r-dbi/DBI/blob/6382f918a56b78eb832348e0eedb538a6721286a/DESCRIPTION#L22-L23
pak::pkg_deps_tree("DBI")
#> ℹ Loading metadata database
#> ✔ Loading metadata database ... done
#>
#> DBI 1.2.2 ✨
#>
#> Key: ✨ new
pak::pkg_deps_tree("DBI", dependencies = TRUE)
#> DBI 1.2.2 ✨
#> ├─arrow 15.0.1 ✨👷🏽♀️🔧 ⬇ (4.37 MB)
#> │ ├─assertthat 0.2.1 ✨ ⬇ (53.25 kB)
#> │ ├─bit64 4.0.5 ✨🔧
#> │ │ └─bit 4.0.5 ✨🔧
#> │ ├─glue 1.7.0 ✨🔧
#> │ ├─purrr 1.0.2 ✨🔧
#> │ │ ├─cli 3.6.2 ✨🔧
#> │ │ ├─lifecycle 1.0.4 ✨
#> │ │ │ ├─cli
#> │ │ │ ├─glue
#> │ │ │ └─rlang 1.1.3 ✨🔧
#> │ │ ├─magrittr 2.0.3 ✨🔧
#> │ │ ├─rlang
#> │ │ └─vctrs 0.6.5 ✨🔧
#> │ │ ├─cli
#> │ │ ├─glue
#> │ │ ├─lifecycle
#> │ │ └─rlang
#> │ ├─R6 2.5.1 ✨
#> │ ├─rlang
#> │ ├─tidyselect 1.2.1 ✨🔧 ⬇ (224.10 kB)
#> │ │ ├─cli
#> │ │ ├─glue
#> │ │ ├─lifecycle
#> │ │ ├─rlang
#> │ │ ├─vctrs
#> │ │ └─withr 3.0.0 ✨
#> │ ├─vctrs
#> │ └─cpp11 0.4.7 ✨ ⬇ (280.86 kB)
#> ├─blob 1.2.4 ✨
#> │ ├─rlang
#> │ └─vctrs
#> ├─covr 3.6.4 ✨🔧 ⬇ (332.09 kB)
#> │ ├─digest 0.6.35 ✨🔧 ⬇ (352.09 kB)
#> │ ├─jsonlite 1.8.8 ✨🔧
#> │ ├─rex 1.2.1 ✨ ⬇ (123.81 kB)
#> │ │ └─lazyeval 0.2.2 ✨🔧
#> │ ├─httr 1.4.7 ✨
#> │ │ ├─curl 5.2.1 ✨🔧 ⬇ (813.53 kB)
#> │ │ ├─jsonlite
#> │ │ ├─mime 0.12 ✨🔧
#> │ │ ├─openssl 2.1.2 ✨🔧
#> │ │ │ └─askpass 1.2.0 ✨🔧
#> │ │ │ └─sys 3.4.2 ✨🔧
#> │ │ └─R6
#> │ ├─crayon 1.5.2 ✨
#> │ ├─withr
#> │ └─yaml 2.3.8 ✨🔧
#> ├─DBItest 1.8.1 ✨ ⬇ (1.95 MB)
#> │ ├─blob
#> │ ├─callr 3.7.6 ✨ ⬇ (437.50 kB)
#> │ │ ├─processx 3.8.4 ✨🔧 ⬇ (317.66 kB)
#> │ │ │ ├─ps 1.7.6 ✨🔧
#> │ │ │ └─R6
#> │ │ └─R6
#> │ ├─DBI
#> │ ├─desc 1.4.3 ✨
#> │ │ ├─cli
#> │ │ └─R6
#> │ ├─hms 1.1.3 ✨
#> │ │ ├─lifecycle
#> │ │ ├─pkgconfig 2.0.3 ✨
#> │ │ ├─rlang
#> │ │ └─vctrs
#> │ ├─lubridate 1.9.3 ✨🔧
#> │ │ ├─generics 0.1.3 ✨
#> │ │ └─timechange 0.3.0 ✨🔧
#> │ │ └─cpp11
#> │ ├─magrittr
#> │ ├─nanoarrow 0.4.0.1 ✨🔧 ⬇ (444.85 kB)
#> │ ├─palmerpenguins 0.1.1 ✨ ⬇ (3.00 MB)
#> │ ├─rlang
#> │ ├─testthat 3.2.1.1 ✨🔧
#> │ │ ├─brio 1.1.4 ✨🔧
#> │ │ ├─callr
#> │ │ ├─cli
#> │ │ ├─desc
#> │ │ ├─digest
#> │ │ ├─evaluate 0.23 ✨
#> │ │ ├─jsonlite
#> │ │ ├─lifecycle
#> │ │ ├─magrittr
#> │ │ ├─pkgload 1.3.4 ✨ ⬇ (180.17 kB)
#> │ │ │ ├─cli
#> │ │ │ ├─crayon
#> │ │ │ ├─desc
#> │ │ │ ├─fs 1.6.3 ✨🔧
#> │ │ │ ├─glue
#> │ │ │ ├─pkgbuild 1.4.4 ✨ ⬇ (202.24 kB)
#> │ │ │ │ ├─callr
#> │ │ │ │ ├─cli
#> │ │ │ │ ├─desc
#> │ │ │ │ ├─processx
#> │ │ │ │ └─R6
#> │ │ │ ├─rlang
#> │ │ │ ├─rprojroot 2.0.4 ✨
#> │ │ │ └─withr
#> │ │ ├─praise 1.0.0 ✨ ⬇ (16.52 kB)
#> │ │ ├─processx
#> │ │ ├─ps
#> │ │ ├─R6
#> │ │ ├─rlang
#> │ │ ├─waldo 0.5.2 ✨ ⬇ (103.51 kB)
#> │ │ │ ├─cli
#> │ │ │ ├─diffobj 0.3.5 ✨🔧 ⬇ (1.02 MB)
#> │ │ │ │ └─crayon
#> │ │ │ ├─fansi 1.0.6 ✨🔧
#> │ │ │ ├─glue
#> │ │ │ ├─rematch2 2.1.2 ✨
#> │ │ │ │ └─tibble 3.2.1 ✨🔧
#> │ │ │ │ ├─fansi
#> │ │ │ │ ├─lifecycle
#> │ │ │ │ ├─magrittr
#> │ │ │ │ ├─pillar 1.9.0 ✨
#> │ │ │ │ │ ├─cli
#> │ │ │ │ │ ├─fansi
#> │ │ │ │ │ ├─glue
#> │ │ │ │ │ ├─lifecycle
#> │ │ │ │ │ ├─rlang
#> │ │ │ │ │ ├─utf8 1.2.4 ✨🔧
#> │ │ │ │ │ └─vctrs
#> │ │ │ │ ├─pkgconfig
#> │ │ │ │ ├─rlang
#> │ │ │ │ └─vctrs
#> │ │ │ ├─rlang
#> │ │ │ └─tibble
#> │ │ └─withr
#> │ └─withr
#> ├─dbplyr 2.5.0 ✨ ⬇ (1.23 MB)
#> │ ├─blob
#> │ ├─cli
#> │ ├─DBI
#> │ ├─dplyr 1.1.4 ✨🔧
#> │ │ ├─cli
#> │ │ ├─generics
#> │ │ ├─glue
#> │ │ ├─lifecycle
#> │ │ ├─magrittr
#> │ │ ├─pillar
#> │ │ ├─R6
#> │ │ ├─rlang
#> │ │ ├─tibble
#> │ │ ├─tidyselect
#> │ │ └─vctrs
#> │ ├─glue
#> │ ├─lifecycle
#> │ ├─magrittr
#> │ ├─pillar
#> │ ├─purrr
#> │ ├─R6
#> │ ├─rlang
#> │ ├─tibble
#> │ ├─tidyr 1.3.1 ✨🔧
#> │ │ ├─cli
#> │ │ ├─dplyr
#> │ │ ├─glue
#> │ │ ├─lifecycle
#> │ │ ├─magrittr
#> │ │ ├─purrr
#> │ │ ├─rlang
#> │ │ ├─stringr 1.5.1 ✨
#> │ │ │ ├─cli
#> │ │ │ ├─glue
#> │ │ │ ├─lifecycle
#> │ │ │ ├─magrittr
#> │ │ │ ├─rlang
#> │ │ │ ├─stringi 1.8.3 ✨🔧
#> │ │ │ └─vctrs
#> │ │ ├─tibble
#> │ │ ├─tidyselect
#> │ │ ├─vctrs
#> │ │ └─cpp11
#> │ ├─tidyselect
#> │ ├─vctrs
#> │ └─withr
#> ├─downlit 0.4.3 ✨
#> │ ├─brio
#> │ ├─desc
#> │ ├─digest
#> │ ├─evaluate
#> │ ├─fansi
#> │ ├─memoise 2.0.1 ✨
#> │ │ ├─rlang
#> │ │ └─cachem 1.0.8 ✨🔧
#> │ │ ├─rlang
#> │ │ └─fastmap 1.1.1 ✨🔧
#> │ ├─rlang
#> │ ├─vctrs
#> │ ├─withr
#> │ └─yaml
#> ├─dplyr
#> ├─glue
#> ├─hms
#> ├─knitr 1.46 ✨
#> │ ├─evaluate
#> │ ├─highr 0.10 ✨
#> │ │ └─xfun 0.43 ✨🔧 ⬇ (488.05 kB)
#> │ ├─xfun
#> │ └─yaml
#> ├─magrittr
#> ├─nanoarrow
#> ├─RMariaDB 1.3.1 ✨👷🏽♀️🔧 ⬇ (908.03 kB)
#> │ ├─bit64
#> │ ├─blob
#> │ ├─DBI
#> │ ├─hms
#> │ ├─lubridate
#> │ ├─rlang
#> │ ├─cpp11
#> │ └─plogr 0.2.0 ✨ ⬇ (13.28 kB)
#> ├─rmarkdown 2.26 ✨ ⬇ (2.62 MB)
#> │ ├─bslib 0.7.0 ✨ ⬇ (5.33 MB)
#> │ │ ├─base64enc 0.1-3 ✨🔧
#> │ │ ├─cachem
#> │ │ ├─fastmap
#> │ │ ├─htmltools 0.5.8.1 ✨🔧 ⬇ (359.58 kB)
#> │ │ │ ├─base64enc
#> │ │ │ ├─digest
#> │ │ │ ├─fastmap
#> │ │ │ └─rlang
#> │ │ ├─jquerylib 0.1.4 ✨
#> │ │ │ └─htmltools
#> │ │ ├─jsonlite
#> │ │ ├─lifecycle
#> │ │ ├─memoise
#> │ │ ├─mime
#> │ │ ├─rlang
#> │ │ └─sass 0.4.9 ✨🔧 ⬇ (2.41 MB)
#> │ │ ├─fs
#> │ │ ├─rlang
#> │ │ ├─htmltools
#> │ │ ├─R6
#> │ │ └─rappdirs 0.3.3 ✨🔧
#> │ ├─evaluate
#> │ ├─fontawesome 0.5.2 ✨
#> │ │ ├─rlang
#> │ │ └─htmltools
#> │ ├─htmltools
#> │ ├─jquerylib
#> │ ├─jsonlite
#> │ ├─knitr
#> │ ├─tinytex 0.50 ✨ ⬇ (139.62 kB)
#> │ │ └─xfun
#> │ ├─xfun
#> │ └─yaml
#> ├─rprojroot
#> ├─RSQLite 2.3.6 ✨🔧 ⬇ (4.37 MB)
#> │ ├─bit64
#> │ ├─blob
#> │ ├─DBI
#> │ ├─memoise
#> │ ├─pkgconfig
#> │ ├─rlang
#> │ ├─plogr
#> │ └─cpp11
#> ├─testthat
#> ├─vctrs
#> └─xml2 1.3.6 ✨🔧
#> ├─cli
#> └─rlang
#>
#> Key: ✨ new | ⬇ download | 👷🏽♀️ build | 🔧 compile
jdhoffa
commented
6 months ago Not entirely related, but do we even need DBI at all anymore? even in suggests?
cjyetman
commented
6 months ago Not entirely related, but do we even need
DBIat all anymore? even in suggests?
yes, it's required to do the SQLite exports, e.g. https://github.com/RMI-PACTA/workflow.data.preparation/blob/5a96cc36b9d343b66bef2d518c6fb340eb686979/run_pacta_data_preparation.R#L654-L670
jdhoffa
commented
6 months ago Got it! Makes sense
Given that we (likely) will want to be able to run the SQLite export using Docker... it seems like we do in fact need that package (and corresponding system) dependency?
Maybe we need to move that to Depends?
Or otherwise just limit SQLite output generation to local run only
cjyetman
commented
6 months ago DBI is in Imports. Do you mean move it to Suggests?
https://github.com/RMI-PACTA/workflow.data.preparation/blob/5a96cc36b9d343b66bef2d518c6fb340eb686979/DESCRIPTION#L30-L32
I thought we had decided that SQLite export should be default for "production" moving forward? https://github.com/RMI-PACTA/workflow.data.preparation/blob/5a96cc36b9d343b66bef2d518c6fb340eb686979/config.yml#L20
Maybe additional R packages that are not technically needed here are already installed in the base image, and the action should be doing this instead?
req_r_pkgs <- sort(unique(pak::pkg_deps(pkg = ".")$package))
pak::sysreqs_check_installed(packages = req_r_pkgs)AH sorry I misunderstood, I thought DBI was in Suggests for THIS repo. But you're saying that the package arrow is in Suggests for DBI, and that's why we are picking it up??
In that case, that sorta seems excessive? I doubt we need the sysdeps of suggested packages of our pkg dependencies here? What do you two think
cjyetman
commented
6 months ago AH sorry I misunderstood, I thought
DBIwas inSuggestsfor THIS repo. But you're saying that the packagearrowis inSuggestsforDBI, and that's why we are picking it up??In that case, that sorta seems excessive? I doubt we need the sysdeps of suggested packages of our pkg dependencies here? What do you two think
Not 100% sure, but it was a suspicion. Have been discussing with pak dev, and it seems like I was wrong. Now I think what is most likely is that arrow is installed in the base image, and because the action is running pak::sysreqs_check_installed(), it's checking for the system dependencies of every installed R package, which is why I'm suggesting to maybe restrict that to...
req_r_pkgs <- sort(unique(pak::pkg_deps(pkg = ".")$package))
pak::sysreqs_check_installed(packages = req_r_pkgs)i.e. check that the system requirements for only the required (not installed) R packages are installed
jdhoffa
commented
6 months ago Indeed you are correct:
cjyetman
commented
6 months ago confirmed
with
FROM --platform=linux/amd64 rocker/tidyverse
RUN Rscript -e 'install.packages("pak")'
RUN git clone https://github.com/RMI-PACTA/workflow.data.preparation.git
RUN Rscript -e 'pak::pak("knitr")'
WORKDIR /workflow.data.preparation# Rscript -e 'pak::sysreqs_check_installed()'
system package installed required by
-------------- -- -----------
cmake ✖ arrow
git ✔ credentials, gitcreds, remotes
libcurl4-openssl-dev ✔ arrow, curl
libfontconfig1-dev ✔ systemfonts
libfreetype6-dev ✔ ragg, systemfonts, textshaping
libfribidi-dev ✔ textshaping
libgit2-dev ✔ gert
libharfbuzz-dev ✔ textshaping
libicu-dev ✔ stringi
libjpeg-dev ✔ ragg
libmysqlclient-dev ✔ RMariaDB
libpng-dev ✔ ragg
libpq-dev ✔ RPostgres
libssl-dev ✔ arrow, curl, openssl
libtiff-dev ✔ ragg
libxml2-dev ✔ xml2
make ✔ fs, haven, httpuv, sass
pandoc ✔ knitr, pkgdown, reprex, rmarkdown
zlib1g-dev ✔ haven, httpuv
# Rscript -e 'req_r_pkgs <- sort(unique(pak::pkg_deps(pkg = ".")$package)); pak::sysreqs_check_installed(packages = req_r_pkgs)'
! Using bundled GitHub PAT. Please add your own PAT using `gitcreds::gitcreds_set()`.
✔ Loading metadata database ... done
system package installed required by
-------------- -- -----------
git ✔ credentials, gitcreds, remotes
libcurl4-openssl-dev ✔ curl
libfontconfig1-dev ✔ systemfonts
libfreetype6-dev ✔ ragg, systemfonts, textshaping
libfribidi-dev ✔ textshaping
libgit2-dev ✔ gert
libharfbuzz-dev ✔ textshaping
libicu-dev ✔ stringi
libjpeg-dev ✔ ragg
libpng-dev ✔ ragg
libssl-dev ✔ curl, openssl
libtiff-dev ✔ ragg
libxml2-dev ✔ xml2
make ✔ fs, httpuv, sass
pandoc ✔ knitr, pkgdown, rmarkdown
zlib1g-dev ✔ httpuv Was my confirmation insufficient? 😭
cjyetman
commented
6 months ago Was my confirmation insufficient? 😭
confirmed that going with this is probably more ideal (and doesn't have the same problem)
req_r_pkgs <- sort(unique(pak::pkg_deps(pkg = ".")$package))
pak::sysreqs_check_installed(packages = req_r_pkgs)Corresponding issue opened here: https://github.com/RMI-PACTA/actions/issues/79
jdhoffa
commented
6 months ago Was my confirmation insufficient? 😭
confirmed that going with this is probably more ideal (and doesn't have the same problem)
req_r_pkgs <- sort(unique(pak::pkg_deps(pkg = ".")$package)) pak::sysreqs_check_installed(packages = req_r_pkgs)
lol, i thought you were just being petty because I used a screenshot 😂
cjyetman
commented
6 months ago superseded by #228
Closes: #220