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

Add support for Artifactory archive types #583

Open
cfmack opened this issue Nov 13, 2019 · 8 comments · May be fixed by #730
Open

Add support for Artifactory archive types #583

cfmack opened this issue Nov 13, 2019 · 8 comments · May be fixed by #730

Comments

@cfmack
Copy link

cfmack commented Nov 13, 2019

In RTFACT-17392, Artifactory has a bug dealing with how CRAN archives. While that is indeed a bug, I have found in 3.5.1 install.packages, varying implementation of REMOTES, and/or versions of packrat, each package attempt to solve these varying implementations differently. Some need to reference a "archive.rds". Others must have a package.rds while others sufficiently use a PACKAGE file.

The ask and the associated PR is allowing the loosening of these restrictions to be backwards compatible. Remotes has a similar bug and PR

@pyltime
Copy link

pyltime commented Jun 8, 2023

This has come up with a customer using Posit Connect in support ticket 90580.

@kevinushey
Copy link
Contributor

We have a solution for this on the renv side; perhaps we could port the relevant code to Packrat? That code lives here: https://github.com/rstudio/renv/blob/029c4457c0c2846eafd9a1da8bf3f95daf49fe45/R/retrieve.R#L806-L860

@jimhester
Copy link
Contributor

The recent version of Connect which bundles a newer version of packrat exposed us to this issue as well. It would be great to either adapt the code from the linked PR or the linked renv solution to support this.

In the short term I have patched the vendored version of packrat for our use, but it would be great to avoid this workaround and use the one bundled with Connect.

@aronatkins
Copy link
Contributor

@jimhester - the implementation of downloadWithRetries() has changed since #584; do you have an alternate patch that you have been using? Would you be able to share it?

@aronatkins
Copy link
Contributor

Related: recent versions of renv assume that Artifactory uses a CRAN-like repository layout, but that may not always be appropriate. rstudio/renv#1996

@jimhester
Copy link
Contributor

jimhester commented Jan 31, 2025

Sure, here's the diff, it is based off of https://github.com/rstudio/packrat/tree/a71a5feebb4f4a9fe95c6421f30b5e51424b1863 that fixes the issue for our use. I unfortunately don't recall why I based it off of that particular commit.

--- packrat-a71a5feebb4f4a9fe95c6421f30b5e51424b1863/R/restore.R	2023-10-18 16:41:38
+++ packrat-nflx/R/restore.R	2023-11-17 10:33:09
@@ -199,10 +199,16 @@
       # each named repository.
       foundVersion <- FALSE
       for (repo in repos) {
+        for (archiveUrl in c(
+          file.path(repo, "src/contrib/Archive",
+            pkgRecord$name,
+            pkgSrcFile),
+          file.path(repo, "src/contrib/Archive",
+            pkgRecord$name,
+            pkgRecord$version,
+            pkgSrcFile)
+          )) {
         tryCatch({
-          archiveUrl <- file.path(repo, "src/contrib/Archive",
-                                  pkgRecord$name,
-                                  pkgSrcFile)
           downloadWithRetries(archiveUrl,
                               destfile = file.path(pkgSrcDir, pkgSrcFile),
                               mode = "wb", quiet = TRUE)
@@ -212,7 +218,7 @@
         }, error = function(e) {
           # Ignore error and try the next repository
         })
-      }
+      }}
       if (!foundVersion) {
         message("FAILED")
         stopMsg <- sprintf("Failed to retrieve package sources for %s %s from CRAN (internet connectivity issue?)",

@aronatkins
Copy link
Contributor

Thanks, @jimhester. I'm surprised that you don't need an additional break with the second loop. I'll push a PR shortly that's similar.

The commit you referenced is the one we pinned before switching to 7062580, which has some improvements when many restores happen concurrently.

aronatkins added a commit that referenced this issue Jan 31, 2025
this is not the most efficient approach, but attempts multiple repository
archive layouts before giving up.

reduces the number of download attempts before giving up. generally, a single
retry is enough. also reduces the cost of additional layout attempts.

fixes #583
@aronatkins aronatkins linked a pull request Jan 31, 2025 that will close this issue
@aronatkins
Copy link
Contributor

This will probably do it: #730

I want to add some tests before merging, too.

aronatkins added a commit that referenced this issue Jan 31, 2025
this is not the most efficient approach, but attempts multiple repository
archive layouts before giving up.

reduces the number of download attempts before giving up. generally, a single
retry is enough. also reduces the cost of additional layout attempts.

fixes #583
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants