Skip to content
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

Fixes and improvements for strategies #26

Merged
merged 28 commits into from
Jul 24, 2023
Merged
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
d86414e
(min_isolatd): enforce Rcpp min version and select min version duplicate
averissimo Jul 10, 2023
aad9662
docs: typo
averissimo Jul 10, 2023
261ed9b
fix: minor bugs and keeps only minimum version of packages
averissimo Jul 10, 2023
c7d2368
fix: typo
averissimo Jul 10, 2023
81dcef0
remove duplicate packages with same version
averissimo Jul 10, 2023
3edf4e6
fix: keep primary dependency version
averissimo Jul 10, 2023
37b6748
docs: move function to utils
averissimo Jul 10, 2023
9821383
fix: improve code on keeping primary dependency version
averissimo Jul 10, 2023
c9f98e5
fix: keep primary dependency min version
averissimo Jul 11, 2023
4352850
remove Rcpp enforment
averissimo Jul 11, 2023
d940853
revert minimal version enforcement
averissimo Jul 11, 2023
0579af9
feat: shows error log from package building
averissimo Jul 11, 2023
c2d4b99
fix: account for multiple dates from gh query
averissimo Jul 12, 2023
89f3b76
feat: supports github only dependencies
averissimo Jul 12, 2023
f65c8b3
docs: remove documentation for deleted function
averissimo Jul 13, 2023
e58166a
fix: improve on graphql query and adds unit test
averissimo Jul 14, 2023
33e7277
chore: adds documentation and compare dates that come from GH GraphQL
averissimo Jul 14, 2023
846f5b0
chore: adds if clause for examples
averissimo Jul 14, 2023
a470d97
fix: solution for Remotes conflicts in version and key (ref)
averissimo Jul 17, 2023
9796b12
fix: Applied Remotes cleanup to min_cohort too
averissimo Jul 17, 2023
5a3a6d9
Adds verbose message to exception that is not self-descriptive
averissimo Jul 18, 2023
0f6b80e
Update R/get_ref.R
averissimo Jul 18, 2023
ac12fea
PR feedback: useless expression corrected, code/msg improvement
averissimo Jul 18, 2023
31ff448
text: Improves on message
averissimo Jul 18, 2023
5370242
fix: Avoid identical calls to resolve_ppm_snapshot
averissimo Jul 19, 2023
7ae7b72
cleanup: remove parent column
averissimo Jul 19, 2023
109db00
chore: replace tab with spaces
averissimo Jul 19, 2023
047d3f2
fix: keep only top version on resolution table to correct non-converg…
averissimo Jul 20, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 22 additions & 37 deletions R/check.R
Original file line number Diff line number Diff line change
Expand Up @@ -117,12 +117,11 @@ solve_ip.deps_installation_proposal <- function(ip) {
#' For each direct dependency, resolve that package using PPM snapshot as of release date + 1.
#' Finally, combine resolutions and run solve.
#' @keywords internal
#' @importFrom pkgcache ppm_repo_url
#' @importFrom pkgdepends new_pkg_deps parse_pkg_ref
#' @exportS3Method solve_ip min_isolated_deps_installation_proposal
solve_ip.min_isolated_deps_installation_proposal <- function(ip) { # nolint
ip$resolve()
res <- ip$get_resolution()
res$parent <- NA_character_

deps <- res[1, "deps"][[1]]
## copy op and version to Config\Needs\verdepcheck rows
Expand All @@ -137,46 +136,26 @@ solve_ip.min_isolated_deps_installation_proposal <- function(ip) { # nolint

cli_pb_init("min_isolated", total = nrow(deps))

deps_res <- lapply(
seq_len(nrow(deps)),
function(i) {
i_pkg <- deps[i, "package"]
deps_res <- lapply(seq_len(nrow(deps)), function(i) {
i_pkg <- deps[i, "package"]

cli_pb_update(package = i_pkg, n = 4L)
cli_pb_update(package = i_pkg, n = 4L)

if (i_pkg %in% base_pkgs()) {
return(NULL)
}
if (i_pkg %in% base_pkgs()) return(NULL)

i_op <- deps[i, "op"]
i_op_ver <- deps[i, "version"]

i_ref_str <- deps[i, "ref"]
i_ref <- pkgdepends::parse_pkg_ref(i_ref_str)
resolve_ppm_snapshot(
deps[i, "ref"],
deps[i, "op"],
deps[i, "version"]
)
})

i_ref_minver <- get_ref_min_incl_cran(i_ref, i_op, i_op_ver)
new_res <- do.call(rbind, deps_res)

i_release_date <- get_release_date(i_ref_minver)
new_res <- new_res[!duplicated(new_res[, c("ref", "package", "version")]), ]
pawelru marked this conversation as resolved.
Show resolved Hide resolved

if (is.na(i_release_date)) {
ppm_repo <- file.path(pkgcache::ppm_repo_url(), "latest")
} else {
ppm_repo <- parse_ppm_url(get_ppm_snapshot_by_date(i_release_date))
}

i_pkg_deps <- pkgdepends::new_pkg_deps(
if (inherits(i_ref_minver, "remote_ref_github")) i_ref_minver$ref else i_ref$ref,
config = list(dependencies = "hard", cran_mirror = ppm_repo)
)
suppressMessages(i_pkg_deps$resolve())
i_res <- i_pkg_deps$get_resolution()
i_res$direct <- i_res$directpkg <- FALSE
i_res
}
)

new_res <- rbind(res[1, ], do.call(rbind, deps_res))
new_res <- new_res[!duplicated(new_res), ]
# Keep res at top
new_res <- rbind(res[1, ], new_res)
pawelru marked this conversation as resolved.
Show resolved Hide resolved

ip$.__enclos_env__$private$plan$.__enclos_env__$private$resolution$result <- new_res
ip$solve()
Expand Down Expand Up @@ -293,7 +272,13 @@ download_ip <- function(ip) {
#' @export
install_ip <- function(ip) {
ip$install_sysreqs()
ip$install()
tryCatch(
ip$install(),
error = function(err) {
# Print compilation error when installation fails to help debug
print(err)
stop(err)
})

return(invisible(ip))
}
Expand Down
57 changes: 56 additions & 1 deletion R/deps_installation_proposal.R
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ new_release_deps_installation_proposal <- function(path, # nolint
}
new_refs_str <- vapply(new_refs, `[[`, character(1), "ref")

d <- desc_remotes_cleanup(d, new_refs)
d <- desc_cond_set_refs(d, new_refs_str)

res <- desc_to_ip(d, config)
Expand Down Expand Up @@ -155,6 +156,7 @@ new_min_cohort_deps_installation_proposal <- function(path, # nolint
}
)
new_refs_str <- vapply(new_refs, `[[`, character(1), "ref")
d <- desc_remotes_cleanup(d, new_refs)
d <- desc_cond_set_refs(d, new_refs_str)

# find PPM snapshot
Expand Down Expand Up @@ -260,6 +262,7 @@ new_min_isolated_deps_installation_proposal <- function(path, # nolint
)
new_refs_str <- vapply(new_refs, `[[`, character(1), "ref")

d <- desc_remotes_cleanup(d, new_refs)
d <- desc_cond_set_refs(d, new_refs_str)

res <- desc_to_ip(d, config)
Expand Down Expand Up @@ -299,6 +302,56 @@ get_refs_from_desc <- function(d) {
res[res_idx]
}

#' Replace Remotes in the `desc` that have been resolved to a GitHub tag or are
#' in CRAN
#'
#' Replaces any existing Remotes entry with the resolved GitHub tag from the
#' `new_refs`.
#'
#' It keeps all the existing Remotes that have not been resolved in `new_refs`.
#'
#' @param d (`desc`) DESCRIPTION object
#' @param new_refs (`list`) remote references that have been resolved and are
#' being updated in `Config/Needs/verdepcheck`
#' @keywords internal
desc_remotes_cleanup <- function(d, new_refs) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I still struggle to fully understand what it does and why it is there.

  1. What it does?
    You have the following:
  new_remotes <- c(
    # Keep remotes (if the DESCRIPTION file is correct, this should have no elements)
    d$get_remotes()[!(remotes_pkg %in% new_ref_pkg)],
    # Modified remotes
    new_ref_remote
  )

d$get_remotes()[!(remotes_pkg %in% new_ref_pkg)] is a vector of remotes not present in config/needs/verdepcheck
new_ref_remote is vector with config/needs/verdepcheck refs for packages existing in remotes field
So essentially it's (conditionally) replacing remotes entries with config/needs/verdepcheck entries. OK seems that I get what it does while writing this comment.
UPDATE: I just came up with example where you actually remove some entries so it's not only about replacing one with another. Then I guess we can modify for loop with if (cond1) then NULL else if (cond2) x else y

  1. Implementation:
  • your function changes the order of remotes entries - don't yet know if it's relevant
  • you call $get_remotes() twice - also not a big deal
  • I was probably struggling because of names of var - there is new_refs, new_ref_remote, new_ref_remote and none of them indicates new references for remote field
    How about a simple for loop:
lapply(
  pkgdepends::parse_pkg_refs(d$get_remotes()),
  function(x) {
    if (<some condition>) then x else config_needs_verdepcheck[get_pkg_name(config_needs_verdepcheck) == x$package]
  }
)
  • (connected to the above) Let's call desc_cond_set_refs first and then desc_remotes_cleanup using one argument only indicating desc object. This could potentially simplify naming issue as well as it fits to the role better - you want to replace desc remotes field with another field (that is being extracted from the same desc object and not another argument)
  1. Is it really needed?
    Please have a look at get_refs_from_desc executed at the very beginning of each installation proposal constructor - it is that function which is responsible for preparing correct list of references for further analysis. I would even ask if we need a Remotes at all - as we already / should have everything we need in the config/needs/verdepcheck. That field can potentially have downstream implications so I am inclined to request to always clear it.

# Parse the remotes to retrieve the package names
remotes <- pkgdepends::parse_pkg_refs(d$get_remotes())

# Get the packages defined in remotes
# (making sure that only packages that are already defined here are modified)
remotes_pkg <- vapply(remotes, `[[`, character(1), "package")

# Find which packages of the new_refs are defined in Remotes
new_refs_gh <- vapply(
new_refs,
function(.x) {
isTRUE(.x$package %in% remotes_pkg) && inherits(.x, "remote_ref_github")
},
logical(1)
)
pawelru marked this conversation as resolved.
Show resolved Hide resolved

# New remotes ref to use when replacing
new_ref_remote <- vapply(new_refs[new_refs_gh], `[[`, character(1), "ref")

# Remotes that are not in new_refs are kept, as well as the ones that were
# resolved to be a github repo
d$clear_remotes()

new_ref_pkg <- vapply(new_refs, `[[`, character(1), "package")
new_remotes <- c(
# Keep remotes
d$get_remotes()[!(remotes_pkg %in% new_ref_pkg)],
pawelru marked this conversation as resolved.
Show resolved Hide resolved
# Modified remotes
new_ref_remote
)
Copy link
Contributor

Choose a reason for hiding this comment

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

generally speaking it seems that it's just new_remotes_refs <- setdiff(remotes_refs, new_refs) - i.e. remove remotes that exists in the second fun arg. You seem to do new_remotes_refs <- c(setdiff(remotes_refs, setdiff(new_refs, remotes_refs)), setdiff(new_refs, remotes_refs)) which is a little bit more complex to follow and fully understand. Am I right with my thinking? Is there any reason why we are doing it this way?

Copy link
Contributor Author

@averissimo averissimo Jul 18, 2023

Choose a reason for hiding this comment

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

The idea here is to remove all packages that have been resolved to a CRAN package and modify the ones that resolved to a GitHub tag.

new_remotes_refs <- c(setdiff(remotes_refs, setdiff(new_refs, remotes_refs)), setdiff(new_refs, remotes_refs))

I think the code below better describes the logic.

note: the setdiff should only output a non-empty vector if the DESCRIPTION is badly formed

c(
  setdiff(remotes_refs, new_refs), # should always be empty unless DESCRIPTION is badly formed
  intercept(Filter(is.github.tag, new_refs), remote_refs)
)

Transforming:

# (...)
Imports:
  a,
  b,
  c
Remotes:
  ie/b@*release
  ie/c@*release
Config/Need/verdepcheck:
  ie/a,
  ie/b,
  ie/c

To:

# (...)
Imports:
  a (>= 1.0.0),
  b (>= 2.0.0),
  c (>= 3.0.0)
Remotes:
  b=ie/b@v2.0.0
Config/Need/verdepcheck:
  a,
  b=ie/b,
  c # exists on CRAN, this is done by `desc_cond_set_refs()`


# Return clause without Remotes section
if (is.null(new_remotes) || length(new_remotes) == 0) return(d)
d$set_remotes(new_remotes)
d
}

#' Set `"Config/Needs/verdepcheck"` section into the `desc` object if not empty else clear this section.
#' @keywords internal
desc_cond_set_refs <- function(d, refs) {
Expand Down Expand Up @@ -329,7 +382,9 @@ desc_to_ip <- function(d, config) {
cli_pb_init <- function(type, total, ...) {
cli::cli_progress_bar(
format = paste(
"{cli::pb_spin} Resolving {cli::pb_extra$type} version of {cli::pb_extra$package}",
"{cli::pb_spin} Resolving",
"{cli::style_bold(cli::col_yellow(cli::pb_extra$type))}",
"version of {cli::col_blue(cli::pb_extra$package)}",
"[{cli::pb_current}/{cli::pb_total}] ETA:{cli::pb_eta}"
),
format_done = paste0(
Expand Down
62 changes: 54 additions & 8 deletions R/get_ref.R
Original file line number Diff line number Diff line change
Expand Up @@ -150,8 +150,16 @@ get_ref_min.remote_ref_github <- function(remote_ref, op = "", op_ver = "") {
}
}

new_ref <- sprintf("%s=%s/%s%s", remote_ref$package, remote_ref$username, remote_ref$repo, ref_suffix) # @TODO
pkgdepends::parse_pkg_ref(new_ref)
new_ref <- sprintf(
"%s=%s/%s%s",
remote_ref$package,
remote_ref$username,
remote_ref$repo,
ref_suffix
)
result <- pkgdepends::parse_pkg_ref(new_ref)

result
averissimo marked this conversation as resolved.
Show resolved Hide resolved
}

# Get list of releases if not empty else get list of tags
Expand All @@ -163,6 +171,7 @@ get_gh_refs <- function(org, repo) {
}
get_gh_tags(org, repo)
}

#' @importFrom gh gh_gql
#' @keywords internal
get_gh_releases <- function(org, repo, max_date = Sys.Date() + 1, min_date = as.Date("1900-01-01")) {
Expand All @@ -187,6 +196,7 @@ get_gh_releases <- function(org, repo, max_date = Sys.Date() + 1, min_date = as.
)
vapply(res, `[[`, character(1), "tagName")
}

#' @importFrom gh gh_gql
#' @keywords internal
get_gh_tags <- function(org, repo, max_date = Sys.Date() + 1, min_date = as.Date("1900-01-01")) {
Expand Down Expand Up @@ -316,40 +326,76 @@ cond_parse_pkg_ref_release <- function(remote_ref) {
get_release_date <- function(remote_ref) {
UseMethod("get_release_date", remote_ref)
}

#' Get release date from GitHub references
#'
#' @inheritParams get_release_date
#'
#' @importFrom gh gh_gql
#' @export
#' @examplesIf gh::gh_token() != ""
#' remote_ref <- pkgdepends::parse_pkg_ref("insightsengineering/[email protected]")
#' get_release_date.remote_ref_github(remote_ref)
get_release_date.remote_ref_github <- function(remote_ref) {
gql_query <- sprintf("{
repository(owner: \"%s\", name: \"%s\") {
refs(refPrefix: \"refs/tags/\", query: \"%s\", first: 100) {
nodes {
target {
... on Commit {
committedDate
edges {
node {
name
target {
... on Commit {
committedDate
}
}
}
}
}
}
}
}", remote_ref$username, remote_ref$repo, remote_ref$commitish)

resp <- try(gh::gh_gql(gql_query), silent = TRUE)
if (inherits(resp, "try-error")) {
return(character(0))
}
vapply(resp$data$repository$refs$nodes, function(x) x$target$committedDate, character(1))

result <- vapply(
resp$data$repository$refs$edges,
function(x) {
if (x$node$name != remote_ref$commitish) return(NA_character_)
x$node$target$committedDate
},
character(1)
)

if (length(result) <= 1) {
return(result %||% NA_character_)
}

max(result, na.rm = TRUE)
}

#' Get release date from GitHub references
#'
#' @inheritParams get_release_date
#'
#' @export
#' @examplesIf Sys.getenv("R_USER_CACHE_DIR", "") != ""
#' remote_ref <- pkgdepends::parse_pkg_ref("[email protected]")
#' get_release_date.remote_ref_cran(remote_ref)
get_release_date.remote_ref_cran <- function(remote_ref) {
subset(
get_cran_data(remote_ref$package),
package_version(version, strict = FALSE) == package_version(remote_ref$version, strict = FALSE),
mtime
)[[1]][1]
}

#' @export
get_release_date.remote_ref_standard <- function(remote_ref) {
get_release_date.remote_ref_cran(remote_ref)
}

#' @export
get_release_date.remote_ref <- function(remote_ref) {
NA
Expand Down
40 changes: 39 additions & 1 deletion R/utils.R
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,10 @@ base_pkgs <- function() {
#' @importFrom pkgcache ppm_snapshots
get_ppm_snapshot_by_date <- function(date) {
snaps <- pkgcache::ppm_snapshots()
res <- as.character(as.Date(head(snaps[as.Date(snaps$date) > as.Date(date), "date"], 1)))
res <- as.character(as.Date(head(
snaps[as.Date(snaps$date) > as.Date(date), "date"],
1
)))
if (length(res) == 0) stop(sprintf("Cannot find PPM snapshot for date after %s.", as.character(date)))
res
}
Expand All @@ -31,3 +34,38 @@ get_ppm_snapshot_by_date <- function(date) {
parse_ppm_url <- function(snapshot) {
file.path(pkgcache::ppm_repo_url(), snapshot)
}

#' Resolve the dependencies of package based on the release date + 1
#'
#' @keywords internal
#' @importFrom pkgcache ppm_repo_url
#' @importFrom pkgdepends new_pkg_deps parse_pkg_ref
resolve_ppm_snapshot <- function(pkg_ref_str, operator, pkg_version) {

i_ref <- pkgdepends::parse_pkg_ref(pkg_ref_str)

i_ref_minver <- get_ref_min_incl_cran(i_ref, operator, pkg_version)

i_release_date <- get_release_date(i_ref_minver)

if (all(is.na(i_release_date))) {
ppm_repo <- file.path(pkgcache::ppm_repo_url(), "latest")
} else {
ppm_repo <- parse_ppm_url(get_ppm_snapshot_by_date(i_release_date))
}

i_pkg_deps <- pkgdepends::new_pkg_deps(
ifelse(
inherits(i_ref_minver, "remote_ref_github"),
i_ref_minver$ref,
i_ref$ref
),
config = list(dependencies = "hard", cran_mirror = ppm_repo)
pawelru marked this conversation as resolved.
Show resolved Hide resolved
)
suppressMessages(i_pkg_deps$resolve())

i_res <- i_pkg_deps$get_resolution()
i_res$direct <- i_res$directpkg <- FALSE
i_res$parent <- pkg_ref_str
i_res
}
23 changes: 23 additions & 0 deletions man/desc_remotes_cleanup.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

20 changes: 20 additions & 0 deletions man/get_release_date.remote_ref_cran.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading