r-lib / pkgdepends

R Package Dependency Resolution
https://r-lib.github.io/pkgdepends/
Other
102 stars 30 forks source link

`pkg_deps_tree()`: Circular dependencies with Remotes cause error #229

Closed wch closed 3 years ago

wch commented 3 years ago

We have a "soft" circular dependency, where shiny suggests shinytest, and shinytest imports shiny. They each list the other in the Remotes: field, since we're using dev versions from github. This causes an error in our CI workflow: https://github.com/rstudio/shiny/runs/1601512829?check_suite_focus=true#step:6:32

Here's the error when I run locally in the Shiny repo directory:

> pak::pkg_deps_tree("local::.", dependencies = TRUE)
Error: Cannot install packages:                                
* local::.: Can't install dependency rstudio/shinytest
* rstudio/shinytest: Can't install dependency rstudio/shiny
* rstudio/shiny: Conflicts with local::.
gaborcsardi commented 3 years ago

Circular dependencies are fine, but you told pak to install shiny from that local directory, and shinytest needs shiny from rstudio/shiny at GitHub, and pak has no way to know that these two are the same, hence the conflict.

If you remove the Remotes field from shinytest, then it will work.

I will have to think what to do to resolve the actual issue.

wch commented 3 years ago

shinytest actually requires the dev version of shiny, though, so we can't remove it from the Remotes.

We're just copying the pak commands from here: https://github.com/r-lib/actions/blob/7216d30a5426859594de6d571f6a3cd065f6d840/examples/check-pak.yaml#L58

If there's some other set of commands that would work better, I'd be happy to switch to it.

gaborcsardi commented 3 years ago

shinytest actually requires the dev version of shiny, though, so we can't remove it from the Remotes.

pak will install the shiny from the local dir, so it will have the dev version of Shiny, at least on GHA.

wch commented 3 years ago

Right, that would fix the GHA errors, but other shinytest users will still need shiny listed in Remotes.

gaborcsardi commented 3 years ago

Sure, it is only a workaround for GHA.

I would think that not too many people use dev shinytest, so it might be OK to do this. Especially if shinytest specifies a shiny version, because then they will learn that dev shiny is needed.

gaborcsardi commented 3 years ago

reprex with pkgdepends:

lib <- tempfile()
p <- pkgdepends::new_pkg_deps("local::.", config = list(dependencies = TRUE, library = lib))
p$resolve()
p$solve()
p$get_solution()
#> <pkg_solution>
#> + result: FAILED
#> + refs:
#>   - local::.
#> + constraints (515):
#>   - select shiny exactly once
#>   - select BH at most once
#>   - select Cairo at most once
#>   - select MASS at most once
#>   - select Matrix at most once
#>   - select R6 at most once
#>   - select RColorBrewer at most once
#>   - select Rcpp at most once
#>   - select assertthat at most once
#>   - select base64enc at most once
#>   ...
#> x failures:
#> * local::.: Can't install dependency rstudio/shinytest
#> * rstudio/shinytest: Can't install dependency rstudio/shiny
#> * rstudio/shiny: Conflicts with local::.

We could probably inspect the local directory and if it is a git repo, then use the sha of the last commit to decide if it is the same as the requested GH remote. This at least solves the GHA issue.

gaborcsardi commented 3 years ago

We could probably inspect the local directory and if it is a git repo, then use the sha of the last commit to decide if it is the same as the requested GH remote.

OTOH this is pretty fragile, e.g. if there is a new commit in rstudio/shiny while the previous CI jobs is starting up (or you are re-running an older build manually), then the solver will still fail.

So I think we'll just assume that if you are installing local::. then you know what you are doing, and that local::. satisfies all dependencies on the local package.

gaborcsardi commented 3 years ago

@wch Once these jobs are done: https://github.com/r-lib/pak/actions?query=workflow%3A%22Build+pak+binary%22 you can try the fix. Should take about 30 minutes.

wch commented 3 years ago

Looks like it worked!