Repository navigation
assess_reverse_dependencies(): don't depend on devtools::revdep() - #401
Draft
aclark02-arcus wants to merge 3 commits into
Draft
aclark02-arcus wants to merge 3 commits into
aclark02-arcus wants to merge 3 commits into
Conversation
devtools::revdep(bioconductor = TRUE) — the previous
implementation of assess_reverse_dependencies.default() —
internally calls devtools:::bioc_packages(), which
unconditionally reads a VIEWS file from
BiocManager::repositories()[['BioCsoft']]. On air-gapped
mirrors (for example, an internal Posit Package Manager
snapshot that only serves <repo>/src/contrib/PACKAGES), that
read fails with:
cannot open the connection to
'<mirror>/bioc-<snapshot>/latest/VIEWS'
The failure was silently converting the reverse-dependencies
metric to a pkg_metric_error for every package in a run, which
downstream tooling (riskreports) rendered as 'unknown'.
Compute reverse dependencies from utils::available.packages() +
tools::dependsOnPkgs() instead. That uses whatever's on
options('repos'), which works cleanly against both public
CRAN/Bioconductor and internal mirrors — no VIEWS file needed.
Expose three new arguments to make the behaviour explicit and
configurable:
* repos — defaults to getOption('repos')
* dependencies — defaults to c('Depends', 'Imports',
'LinkingTo', 'Suggests') to
match the previous devtools::revdep()
behaviour
* available — optional pre-computed available.packages()
matrix for callers that want to avoid a
repeated network fetch across many packages
Ships with three tests exercising the default expansion, the
dependencies filter, and the empty-result path, plus a NEWS
entry under the development version.
There was a problem hiding this comment.
Pull request overview
This PR refactors assess_reverse_dependencies.default() to avoid relying on devtools::revdep(bioconductor = TRUE) (and its VIEWS fetch), computing reverse dependencies instead from repository PACKAGES metadata via utils::available.packages() + tools::dependsOnPkgs() to improve behavior on air-gapped/internal mirrors.
Changes:
- Reimplemented
assess_reverse_dependencies.default()to compute revdeps from an available-packages index, addingrepos,dependencies, andavailableparameters. - Added
testthatcoverage for the new computation logic (using a suppliedavailablematrix). - Updated
NEWS.mdand removedimportFrom(devtools, revdep)fromNAMESPACE.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
R/assess_reverse_dependencies.R |
Replaces devtools::revdep() with an available.packages()/dependsOnPkgs()-based implementation and documents new parameters. |
tests/testthat/test_assess_reverse_dependencies.R |
Adds unit tests for the revdep computation (primarily via the available= short-circuit). |
NEWS.md |
Documents the behavioral change and rationale (air-gapped mirror compatibility). |
NAMESPACE |
Removes the unused devtools::revdep import. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+59
to
+63
| ap <- if (is.null(available)) { | ||
| utils::available.packages(repos = repos) | ||
| } else { | ||
| available | ||
| } |
Comment on lines
+1
to
+28
| test_that("assess_reverse_dependencies.default computes revdeps from available.packages()", { | ||
| # Build a fake CRAN-style available-packages matrix with three reverse | ||
| # dependents of the target package "target" (via Depends, Imports, and | ||
| # LinkingTo), plus one unrelated package. | ||
| fake_available <- matrix( | ||
| c( | ||
| "revA", "1.0", "target", "", "", "", | ||
| "revB", "2.0", "", "target", "", "", | ||
| "revC", "3.0", "", "", "target", "", | ||
| "unrelated","0.1", "utils", "", "", "" | ||
| ), | ||
| ncol = 6, | ||
| byrow = TRUE, | ||
| dimnames = list(NULL, c("Package", "Version", "Depends", | ||
| "Imports", "LinkingTo", "Suggests")) | ||
| ) | ||
| # Package-level Repository column is required by tools::dependsOnPkgs | ||
| fake_available <- cbind( | ||
| fake_available, | ||
| Repository = "https://example.test/src/contrib" | ||
| ) | ||
|
|
||
| x <- list(name = "target") | ||
| res <- assess_reverse_dependencies.default(x, available = fake_available) | ||
|
|
||
| expect_s3_class(res, "pkg_metric_reverse_dependencies") | ||
| expect_setequal(as.character(res), c("revA", "revB", "revC")) | ||
| }) |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
riskmetric::assess_reverse_dependencies.default()currently calls:devtools::revdepin turn callsdevtools:::bioc_packages()(source), which is memoised and unconditionally does:On an air-gapped host — e.g. an internal Posit Package Manager mirror that only serves
<repo>/src/contrib/PACKAGESand does not expose a<repo>/VIEWSfile at the mirror root — thatread.dcffails with:assess_reverse_dependenciesthen wraps the error into apkg_metric_errorfor every package in the run. Downstream tooling (riskreports) renders that as"unknown", so validation reports coming out of air-gapped environments silently drop reverse-dependency information for every package. This can be perceived as a bug because a fully-configured internal CRAN + BioC mirror is nonetheless unable to answer a question that only needsPACKAGES.Change
Compute the reverse-dependency list from
utils::available.packages()+tools::dependsOnPkgs()instead. This uses the caller'soptions("repos")(or an explicitrepos =vector), which works cleanly against both public CRAN + Bioconductor and internal mirrors — noVIEWSfile needed.Three new arguments make the behaviour explicit and configurable:
repos— defaults togetOption("repos").dependencies— defaults toc("Depends", "Imports", "LinkingTo", "Suggests")to match the previousdevtools::revdep()behaviour.available— an optional pre-computedavailable.packages()matrix, so callers that iterate over many packages in one session can avoid repeated network fetches.Public-network callers get the same result as before (the default
repospicks up whatever's already configured); air-gapped callers can pointoptions(repos = ...)at their internal mirror once and expect the metric to populate correctly.Compatibility
assess_reverse_dependencies()and the return-type contract are unchanged — still apkg_metric_reverse_dependenciesobject carrying a character vector of reverse-dependency package names.devtoolsremains a dependency (still imported bypkg_ref_cache.r_cmd_check), so noDESCRIPTIONchange is needed.importFrom(devtools, revdep)dropped fromNAMESPACE— no longer referenced.@rdnameon the method).Tests
Three new tests in
tests/testthat/test_assess_reverse_dependencies.Rcover:available.packages()matrix with revdeps viaDepends,Imports, andLinkingTo, the metric returns all three revdep package names.dependenciesfilter. Passingdependencies = "Depends"on a matrix whose revdeps live inDepends,Imports, andSuggestsreturns only theDependsrevdep.pkg_metric_reverse_dependencies.All three pass locally.
Notes
Opened as a draft for maintainer discussion. Two things I'd love feedback on:
dependencies.c("Depends", "Imports", "LinkingTo", "Suggests")matches the olddevtools::revdep()default. Reasonable to narrow to"strong"(dropSuggests) if there's a preference — that would change the numericmetric_score()for packages with many Suggests-only revdeps.availableshort-circuit is designed for theriskmetric::pkg_assess()bulk case. Happy to also wire it throughall_assessments()/pkg_assess()so theavailable.packages()call is memoised across the whole run, if that's the preferred pattern here.