From 949e66f2c7604e4569d923622ddbdb8d79c3602a Mon Sep 17 00:00:00 2001 From: maksymis <32574056+maksymiuks@users.noreply.github.com> Date: Thu, 7 Nov 2024 14:12:33 +0100 Subject: [PATCH 1/2] Add development revdep check --- DESCRIPTION | 2 +- NEWS.md | 4 ++ R/check.R | 1 + R/checks_df.R | 61 +++++++++++------ R/task_graph.R | 4 +- man/rev_dep_check_tasks_df.Rd | 4 ++ tests/testthat/test-check-reverse.R | 63 +++++++++++++----- .../revdeps/v1/pkg.none.broken_1.0.0.tar.gz | Bin 0 -> 932 bytes 8 files changed, 100 insertions(+), 39 deletions(-) create mode 100644 tests/testthat/testing_pkgs/revdeps/v1/pkg.none.broken_1.0.0.tar.gz diff --git a/DESCRIPTION b/DESCRIPTION index 5586de7..602da97 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -1,6 +1,6 @@ Package: checked Title: Systematically Run R CMD Checks -Version: 0.2.4 +Version: 0.2.5 Authors@R: c( person( diff --git a/NEWS.md b/NEWS.md index 2922b22..c0eac84 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,3 +1,7 @@ +# checked 0.2.5 + +* Refine reverse suggested dependecy strategy. + # checked 0.2.4 * Fix check processes hanging forever in some system configurations. diff --git a/R/check.R b/R/check.R index 2d5db48..90e5e66 100644 --- a/R/check.R +++ b/R/check.R @@ -66,6 +66,7 @@ check_rev_deps <- function( checks <- rev_dep_check_tasks_df( path = path, repos = reverse_repos, + lib.loc = lib.loc, ... ) diff --git a/R/checks_df.R b/R/checks_df.R index b197c23..25cb404 100644 --- a/R/checks_df.R +++ b/R/checks_df.R @@ -57,6 +57,8 @@ NULL #' mostly when checking whether adding new package would break tests of #' packages already in the repository and take the package as suggests #' dependency. +#' @param lib.loc vector of libraries used to check whether reverse dependency +#' check can return accurate results. #' #' @family tasks #' @export @@ -64,6 +66,7 @@ rev_dep_check_tasks_df <- function( path, repos = getOption("repos"), versions = c("dev", "release"), + lib.loc = .libPaths(), ... ) { stopifnot( @@ -92,26 +95,38 @@ rev_dep_check_tasks_df <- function( version = version ) - if (!package %in% ap[, "Package"] && "release" %in% versions) { - warning( - sprintf( - "Package `%s` not found in repositories `%s`. Skipping 'release' in 'versions'", - package, - paste0(repos, collapse = ", ") - ), - immediate. = TRUE - ) - if ("dev" %in% versions) { - versions <- "dev" + reverse_suggests <- if (!package %in% ap[, "Package"] && "release" %in% versions) { + if (is_package_installed(package, lib.loc)) { + stop( + sprintf( + "Package `%s` not found in repositories `%s` and identified in lib.loc. + checked cannot provide accurate reverse dependency check results. + Remove the package from lib.loc and try again.", + package, + paste0(repos, collapse = ", ") + ) + ) + } else { - return(empty_checks_df) + warning( + sprintf( + "Package `%s` not found in repositories `%s`. Skipping 'release' version of the package", + package, + paste0(repos, collapse = ", ") + ), + immediate. = TRUE + ) } + + TRUE + } else { + FALSE } task_specs_function <- if (all(c("dev", "release") %in% versions)) { rev_dep_check_tasks_specs } else { - rev_dep_check_tasks_specs_development + check_tasks_specs } if ("dev" %in% versions) { @@ -125,15 +140,19 @@ rev_dep_check_tasks_df <- function( } if ("release" %in% versions) { - package_v <- ap[package, "Version"] + package_v <- if (reverse_suggests) "missing" else ap[package, "Version"] df_rel$alias <- paste0(df_rel$alias, " (v", package_v, ")") df_rel$package <- task_specs_function(revdeps, repos, df_rel$alias, "old", ...) - df_rel$custom <- rep(list(custom_install_task_spec( - alias = paste0(package, " (release)"), - package_spec = package_spec(name = package, repos = repos), - # make sure to use the release version built against the same system - type = "source" - )), times = NROW(df_dev)) + df_rel$custom <- if (reverse_suggests) { + rep(list(custom_install_task_spec()), times = NROW(df_dev)) + } else { + rep(list(custom_install_task_spec( + alias = paste0(package, " (release)"), + package_spec = package_spec(name = package, repos = repos), + # make sure to use the release version built against the same system + type = "source" + )), times = NROW(df_dev)) + } } if (identical(versions, "dev")) { @@ -176,7 +195,7 @@ rev_dep_check_tasks_specs <- function(packages, repos, aliases, revdep, ...) { )) } -rev_dep_check_tasks_specs_development <- function( +check_tasks_specs <- function( packages, repos, aliases, diff --git a/R/task_graph.R b/R/task_graph.R index ac04720..b7e46c7 100644 --- a/R/task_graph.R +++ b/R/task_graph.R @@ -329,11 +329,11 @@ task_graph_set_task_process <- function(g, v, process) { task_graph_update_done <- function(g, lib.loc) { v <- igraph::V(g)[igraph::V(g)$type == "install"] - which_done <- which(vlapply(v$name, is_package_done, lib.loc = lib.loc)) + which_done <- which(vlapply(v$name, is_package_installed, lib.loc = lib.loc)) task_graph_set_package_status(g, v[which_done], STATUS$done) } -is_package_done <- function(pkg, lib.loc) { # nolint object_name_linter +is_package_installed <- function(pkg, lib.loc) { # nolint object_name_linter path <- find.package(pkg, lib.loc = lib.loc, quiet = TRUE) length(path) > 0 } diff --git a/man/rev_dep_check_tasks_df.Rd b/man/rev_dep_check_tasks_df.Rd index 9fae60f..d62613b 100644 --- a/man/rev_dep_check_tasks_df.Rd +++ b/man/rev_dep_check_tasks_df.Rd @@ -13,6 +13,7 @@ rev_dep_check_tasks_df( path, repos = getOption("repos"), versions = c("dev", "release"), + lib.loc = .libPaths(), ... ) } @@ -31,6 +32,9 @@ mostly when checking whether adding new package would break tests of packages already in the repository and take the package as suggests dependency.} +\item{lib.loc}{vector of libraries used to check whether reverse dependency +check can return accurate results.} + \item{...}{parameters passed to the task specs allowing to customize subprocesses.} } diff --git a/tests/testthat/test-check-reverse.R b/tests/testthat/test-check-reverse.R index 221b0d0..576a640 100644 --- a/tests/testthat/test-check-reverse.R +++ b/tests/testthat/test-check-reverse.R @@ -101,26 +101,59 @@ test_that("check_rev_deps works for a package without release version", { expect_true(is.list(r)) expect_named(r) expect_length(r, 1L) - expect_length(r$check_task_spec, 1L) - - expect_length(r$check_task_spec$`pkg.none (dev)`$notes$issues, 0L) - expect_length(r$check_task_spec$`pkg.none (dev)`$notes$potential_issues$new, 0L) - expect_length(r$check_task_spec$`pkg.none (dev)`$notes$potential_issues$old, 0L) - - expect_length(r$check_task_spec$`pkg.none (dev)`$warnings$issues, 0L) - expect_length(r$check_task_spec$`pkg.none (dev)`$warnings$potential_issues$new, 0L) - expect_length(r$check_task_spec$`pkg.none (dev)`$warnings$potential_issues$old, 0L) - + expect_length(r$revdep_check_task_spec, 2L) - expect_length(r$check_task_spec$`pkg.none (dev)`$errors$issues, 1L) + # pkg.none + expect_length(r$revdep_check_task_spec$pkg.none$notes$issues, 0L) + expect_length(r$revdep_check_task_spec$pkg.none$notes$potential_issues$new, 0L) + expect_length(r$revdep_check_task_spec$pkg.none$notes$potential_issues$old, 0L) + + expect_length(r$revdep_check_task_spec$pkg.none$warnings$issues, 0L) + expect_length(r$revdep_check_task_spec$pkg.none$warnings$potential_issues$new, 0L) + expect_length(r$revdep_check_task_spec$pkg.none$warnings$potential_issues$old, 0L) + + + expect_length(r$revdep_check_task_spec$pkg.none$errors$issues, 0L) + expect_length(r$revdep_check_task_spec$pkg.none$errors$potential_issues$new, 0L) + expect_length(r$revdep_check_task_spec$pkg.none$errors$potential_issues$old, 0L) + + + # pkg.none.broken + expect_length(r$revdep_check_task_spec$pkg.none.broken$notes$issues, 0L) + expect_length(r$revdep_check_task_spec$pkg.none.broken$notes$potential_issues$new, 0L) + expect_length(r$revdep_check_task_spec$pkg.none.broken$notes$potential_issues$old, 0L) + + expect_length(r$revdep_check_task_spec$pkg.none.broken$warnings$issues, 0L) + expect_length(r$revdep_check_task_spec$pkg.none.broken$warnings$potential_issues$new, 0L) + expect_length(r$revdep_check_task_spec$pkg.none.broken$warnings$potential_issues$old, 0L) + + + expect_length(r$revdep_check_task_spec$pkg.none.broken$errors$issues, 1L) expect_true( grepl("Running the tests in", - r$check_task_spec$`pkg.none (dev)`$errors$issues), + r$revdep_check_task_spec$pkg.none.broken$errors$issues), grepl("\"hello world\" is not TRUE", - r$check_task_spec$`pkg.none (dev)`$errors$issues) + r$revdep_check_task_spec$pkg.none.broken$errors$issues) ) - expect_length(r$check_task_spec$`pkg.none (dev)`$errors$potential_issues$new, 0L) - expect_length(r$check_task_spec$`pkg.none (dev)`$errors$potential_issues$old, 0L) + expect_length(r$revdep_check_task_spec$pkg.none.broken$errors$potential_issues$new, 0L) + expect_length(r$revdep_check_task_spec$pkg.none.broken$errors$potential_issues$old, 0L) + + # Error testing + dir.create(temp_lib <- tempfile("testing_lib")) + install.packages( + file.path(sources_new, "pkg.suggests"), + lib = temp_lib, + type = "source", + repos = NULL + ) + + withr::with_options(list(pkgType = "source"), { + expect_error(design <- check_rev_deps( + file.path(sources_new, "pkg.suggests"), + lib.loc = temp_lib, + n = 2L, repos = repo, env = c("NOT_CRAN" = "false", options::opt("check_envvars"))), + "cannot provide accurate reverse dependency check results") + }) }) test_that("check_dev_rev_deps works as expected", { diff --git a/tests/testthat/testing_pkgs/revdeps/v1/pkg.none.broken_1.0.0.tar.gz b/tests/testthat/testing_pkgs/revdeps/v1/pkg.none.broken_1.0.0.tar.gz new file mode 100644 index 0000000000000000000000000000000000000000..8813c3ccc352cbd082d311ddd27b3e122e82c074 GIT binary patch literal 932 zcmV;V16%wbiwFP!000001MOLVZ{ju>=5K$BcR8Id(I7Y>2`6@)S^;-OqJUDmsoG5^ zml%i$c9s~m2<^M?O@J%(epELxI-xuQlDu{j=e3{5k4Ze6m_iC}-X(IzMYYwwZu*_U zu+#6A4h4$v94Ft_JvZM{dwqH{p&JTbhP)m$S;`Ia z8&A?uiUwFFH7U7RsHsfSbH4!u5HS`@p=9jP%AT6Gbk}y_z z`#wKaym_i1e=2-@j3jK5uy|^iq#KU7NHGgnogw@T;}F}@?KIoH>o#eNkqkmHX~50! zedR6j<->Bq@!CYWS@XPnlc=z=l`IVFkoD!bUzPcIS2ffEL`cJ9rq5gZG<=HNEJk)>3bDcu} zkAd6vg`bOxMVkG=t>5`@IfTo8x7GF^=LBGDB`?T-^8BAOQ9blMa^Ab{xA{-$Q~o=) zRRXu*|BLkhlt+>LbT5-AF#X`a*Z&%|pY(s-w%kJhkAc_lfur|hiU)AFM7y2aRJ(zW z7=5P#9)v0@m@*NN*D#1UI{kGtkfTLrymOZdp#Z-+y*GXGQLs5*?F^#7$wwB2EPWDF z*~(#cnZ;lmTcqf6S7fK6ZV-$IkM4a%c22>vLcc8cDvHqvU z`u{lk{|0vL)T=l6f%CP_@7geUSAp?Dj4}tN8?Yh(+Vt_npR~e>odo|?_8)Zh@cjQV z{ddt57W)5k{h$4QI@m<;tp0oU=l6fq#kl2pv{vZ Date: Wed, 13 Nov 2024 15:28:00 +0100 Subject: [PATCH 2/2] Refine warnings --- R/checks_df.R | 12 +----------- tests/testthat/test-check-reverse.R | 2 +- 2 files changed, 2 insertions(+), 12 deletions(-) diff --git a/R/checks_df.R b/R/checks_df.R index 25cb404..62b033f 100644 --- a/R/checks_df.R +++ b/R/checks_df.R @@ -107,17 +107,7 @@ rev_dep_check_tasks_df <- function( ) ) - } else { - warning( - sprintf( - "Package `%s` not found in repositories `%s`. Skipping 'release' version of the package", - package, - paste0(repos, collapse = ", ") - ), - immediate. = TRUE - ) - } - + } TRUE } else { FALSE diff --git a/tests/testthat/test-check-reverse.R b/tests/testthat/test-check-reverse.R index 576a640..87af16d 100644 --- a/tests/testthat/test-check-reverse.R +++ b/tests/testthat/test-check-reverse.R @@ -86,7 +86,7 @@ test_that("check_rev_deps works for a package without release version", { # Ensure source installation to make sure test works also on mac and windows withr::with_options(list(pkgType = "source"), { - expect_warning(design <- check_rev_deps( + expect_no_warning(design <- check_rev_deps( file.path(sources_new, "pkg.suggests"), n = 2L, repos = repo, env = c("NOT_CRAN" = "false", options::opt("check_envvars")))) })