Closed toph-allen closed 1 year ago
Tests started failing with this commit. I have no idea why the tests that are failing started to fail here, because the failures seem unrelated.
@aronatkins Exhaustive description of download functions in use. Let me know if this checks out.
Could you explain the contract of each of the download functions? In particular: Where do they write, who handles deletion of files on error, and how does the caller gauge success (return value versus error, etc).
In general, I've moved away from having success encoded in return values, and towards leaning entirely on R errors (i.e. stop()
). Occasionally, callers will intercept this to add additional context.
getSourceForPkgRecord
srczip
at this level. It doesn't create the file — the download implementations do that — but it manages its lifecycle, deleting it on exit if it exists.githubDownload(url = archiveUrl, destfile = srczip)
, gitlabDownload(url = archiveUrl, destfile = srczip)
, bitbucketDownload(url = archiveUrl, destfile = srczip)
I'm going sometimes refer to all of these collectively as providerDownload
.
getSourceForPkgRecord
.url
(the place to retrieve the source from) and destfile
(the location to place the retrieved source) to an inner download implementation, depending on various environmental factors.destfile
, called srczip
in the caller — is a temporary file. The caller, getSourceForPkgRecord
, handles its creation and deletion. It is always deleted when getSourceForPkgRecord
exits. (The downloaded source is moved elsewhere after its DESCRIPTION
has the remote info appended.)tryCatch
block. This is so that it can add a note to the error message advising the user to "Check the GITHUB_PAT environment variable." This message is added whatever the underlying error. I kept it because this is what Packrat did before, but I don't necessarily think this is the best thing, as it kinda sounds like we've identified the error and are instructing the user on how to solve it.renvDownload
, providerDownloadHttr
, or downloadWithRetries
. It does not check the output of these functions; it assumes that they will raise an error if they fail.renvDownload(url, destfile, type)
renv$download
providerDownload
.url
and destfile
correspond to archiveUrl
and srczip
respectively. These are their renv
names.type
is a lowercase string "github"
, "gitlab"
, or "bitbucket"
. renv
uses this to determine what type of authentication to use. This is hard-coded, created at the moment that providerDownload
calls renvDownload
.TRUE
if renv$download
did not cause an error.
return (identical(result, destfile))
, but looking at the function now, this doesn't actually seem to be semantically meaningful.result
and destfile
are identical, it should check them. But I don't think that they can diverge. I'm not sure why renv
has these functions return the destfile
path. The only time that it's possible for destfile
to change is replacing backslashes with forward slashes on Windows, which is expected. So I'm going to remove the check.
Now it just returns TRUE
to fit in with Packrat conventions, but this value is not checked at runtime. Expects that renv
will raise an error if something goes wrong.destfile
, which is managed by the top-level function. renv
writes to another temporary file internally in some instances, but we rely on it to clean those up.renv$download
finishes without raising an error, if destfile
does not exist at return time, it will raise the same error that providerDownloadHttr
functions raise in the same condition.githubDownloadHttr(url, destfile, ...)
, gitlabDownloadHttr(url, destfile, ...)
, bitbucketDownloadHttr(url, destfile, ...)
Referring to these singularly/collectively as providerDownloadHttr
.
providerDownload
if its requirements are met and renvDownload
's are not.TRUE
if it succeeds, but also calls stop()
if any errors are encountered.destfile
, is owned by getSourceForPkgRecord
.downloadWithRetries(url, ..., maxTries = 5L)
I didn't touch much of this, except to add an error at the end of the download function wasn't successful.
download
function in a retry pattern. Will raise errors that come from that function, and will also raise an error if the success
marker is not TRUE
after maxTries
attempts.TRUE
if its success marker is TRUE
.destfile
down to download
via ...
.download(url, destfile, method = inferAppropriateDownloadMethod(url)
Down into old packrat code I didn't touch.
destfile
. But it does download to a temporary file that it manages, and that is unlink
ed on exit.status
if status
. This needs to be handled by its caller. (downloadWithRetries
handles it by treating anything except 200
as a failure. But really, this shouldn't be necessary — read on. I think this was to handle an old Windows edge case?)file.rename(tempfile, destfile)
0
if everything was successful or 1
.downloadImpl
method which calls a downloadFile
method, which wraps download.file
. download.file
returns 0
if successful, otherwise a status code, which is how download
ever gets to renaming its tempfile
. Down here, success is 0
and failure is 1
, which is flipped elsewhere up the stack. This is another reason that, at the level I worked at, I wanted to rely on errors rather than exit values.@aronatkins I found a bug in the logic in the githubDownload
etc. functions.
Specifically, the conditional logic is specified such that if GitHub is authenticated but neither renvDownload
nor githubDownloadHttr
are available, downloadWithRetries
is not called as a fallback.
I will submit a fix shortly.
@aronatkins Changes are in these commits.
I explored using the download() { downloadImpl() }
pattern, but found that it would still require a fair amount of boilerplate in the top-level download function, because there is variation on two axes: download method and provider. The downloadRenv()
function would have required different logic from githubDownloadHttr()
, for example, and passing that data around would have made each function call different in the top-level dispatch functions.
What I wound up doing was creating an error handling closure that returns a function based on type ("github"
, "gitlab"
, or "bitbucket"
) to generate error text. It makes the download function logic much clearer than before, with richer error messages that give explicit information about the expected behavior.
I also expanded tests for each provider to cover the expected scenarios, and added tests for the new error message function.
Gonna kick off a validation run to make sure I haven't introduced new bugs.
@aronatkins, @ChaitaC and I just validated this PR together on 2022-10-17.
Intent
renv
code to download packages hosted in private repos on GitHub, GitLab, and Bitbucket.Approach
At a high level, reworked the overall workflow for Git providers in the
getSourceForPkgRecord
function. This involves change to downloader and provider-specific functions.File-by-file notes
Because this involves a lot of code moving around, I took file-by-file notes on changes. These are ordered logically, from highest to lowest-level changes, with links to the files in GitHub's "Files changed" view. Hopefully this makes it easier to navigate.
getSourceForPkgRecord
downloadWithRetries
to account for the error being generated within the function.[provider]ArchiveUrl
function located in each provider's.R
file.[provider]Download
function located in each provider's.R
file. If an error is returned from the download function, a generic message about the failure is added to the error text here.local
block that appended the remote info to each package'sDESCRIPTION
file has been moved to amodifyRemoteInfo
function.modifyRemoteInfo
function houses code that used to exist in each provider workflow.DESCRIPTION
.src
anddest
argument, as well as aremote_info
data frame.src
, appends theremote_info
to theDESCRIPTION
file, and recompresses the package atdest
. This is thus used to move the package to its final location.RemoteType
field. It used not to include this.renv
adds this field in migration from Packrat, and it exists for Bitbucket and GitLab. WithinmodifyRemoteInfo
we use this to determine the field locating the package in a subdirectory of thetar
archive. If adding this field is bad, we could feasibly identify it another way, e.g. see if any of the field names matchGitHubSubdir
. (In fact, writing this, I think this may be the best way to do this.)dest
.tar.gz
.[provider]Download
functions now contain the logic for selecting what download function to use. The download function logic has been refactored. If the PAT is present in the environment and an error is encountered, the error message is edited to include a directive to check the appropriate environment variable.[provider]DownloadImpl
has been renamed to[provider]DownloadHttr
and now just prints thehttr
status message, as the rest of the error messages are provided elsewhere.[provider]ArchiveUrl
function contains the code code that used to live ingetSourceForPkgRecord
.libcurl
as a download method, including alteringsecureDownloadMethod
and removingcanUseLibCurlDownloadMethod
.downloadWithRetries
so that it stops with an error if it fails, instead of simply not returning TRUE.renvDownload
.renv
has internal error handling code which raises errors.renvDownload
.renv.http.useragent
to"curl"
. Sets environmentRENV_DOWNLOAD_METHOD
to themethod
passed into the function.renv$download
, returning TRUE if successful.canUseHttr
function, which really just wraps a check topackageVersionInstalled
but makes other code more readable, maybe?Automated Tests
Multiple automated tests have been added for lower-level functions
renvDownload
passes its arguments through torenv$download
. This is partially so that we can directly mockrenvDownload
in other tests to work around a bug inmockery
that prevents namespaced mocks of nested functions.[provider]ArchiveUrl
functions are correct. More importantly, they also ensure that[provider]Download
passes values through torenvDownload
correctly when the correct options and environment variables are in play.modifyRemoteInfo
is tested.QA Notes
It's difficult to publicly and reproducibly verify end-to-end downloads of private packages. We'll perform verification offline.
Checklist
Note: Some of this is still TODO
NEWS