r-lib / remotes

Install R packages from GitHub, GitLab, Bitbucket, git, svn repositories, URLs
https://remotes.r-lib.org/
Other
332 stars 152 forks source link

exit status 0 when installation fails from nonexistent ref #755

Open infotroph opened 1 year ago

infotroph commented 1 year ago

When the repo exists but the requested tag/branch doesn't, I get error text but apparently no error condition:

Rscript --vanilla -e 'remotes::install_github("r-lib/roxygen2@vFake")'; echo $?
Downloading GitHub repo r-lib/roxygen2@vFake
Error in utils::download.file(url, path, method = method, quiet = quiet,  : 
  cannot open URL 'https://api.github.com/repos/r-lib/roxygen2/tarball/vFake'
0

I'm a bit puzzled because it seems that the underlying download.file call does signal an error condition when it produces the text...

Rscript --vanilla -e 'utils::download.file("https://api.github.com/repos/r-lib/roxygen2/tarball/vFake", destfile = "/tmp/fake")'; echo $?
trying URL 'https://api.github.com/repos/r-lib/roxygen2/tarball/vFake'
Error in utils::download.file("https://api.github.com/repos/r-lib/roxygen2/tarball/vFake",  : 
  cannot open URL 'https://api.github.com/repos/r-lib/roxygen2/tarball/vFake'
In addition: Warning message:
In utils::download.file("https://api.github.com/repos/r-lib/roxygen2/tarball/vFake",  :
  cannot open URL 'https://codeload.github.com/r-lib/roxygen2/legacy.tar.gz/vFake': HTTP status was '404 Not Found'
Execution halted
1

...and I can't immediately see where the condition is intercepted in the intervening function calls.

If y'all agree the exit status should be 1 in this case I'm happy to keep looking and attempt a PR, but I will wait first for confirmation that the existing behavior isn't intended.

Context

I noticed this when a Makefile I maintain failed to update a package (because of a typo in the tag) but then kept running downstream tasks using outdated previously-installed version of the package (because Make saw the exit code from the update was 0 and treated it as success).

Versions

Noticed with remotes 2.4.2, replicated with 2.4.2.9000 commit 1e121409, both in R 4.3.0 on MacOS 12.6.3.

gaborcsardi commented 1 year ago

Yes, the exit status should be 1 for sure, unfortunately install.packages() does not error when it fails to install a package:

❯ Rscript -e 'install.packages("foobar12345")'
Installing package into ‘/Users/gaborcsardi/Library/R/arm64/4.3/library’
(as ‘lib’ is unspecified)
Warning message:
package ‘foobar12345’ is not available for this version of R

A version of this package for your version of R might be available elsewhere,
see the ideas at
https://cran.r-project.org/doc/manuals/r-patched/R-admin.html#Installing-packages

~
❯ echo $?
0
gaborcsardi commented 1 year ago

Hmmm, otoh in this the error happens before we call install.packages().

But install_github() does throw an error, so I am not sure why R returns with a zero status.

Also, you cannot rely on install.packages() and thus you cannot rely on remotes, either, to fail with a non-zero status.

You might want to try r-lib/pak, which is an alternative to remotes, and it does fail properly:

❯ Rscript -e 'pak::pkg_install("r-lib/roxygen2@vFake")'; echo $?
Error:
! error in pak subprocess
Caused by error:
! Could not solve package dependencies:
* r-lib/roxygen2@vFake: ! pkgdepends resolution error for r-lib/roxygen2@vFake.
Caused by error:
! Can't find reference @vFake in GitHub repo r-lib/roxygen2.
---
Backtrace:
1. pak::pkg_install("r-lib/roxygen2@vFake")
2. pak:::remote(function(...) get("pkg_install_make_plan", asNamespace("pak"))(...)…
3. err$throw(res$error)
---
Subprocess backtrace:
1. base::withCallingHandlers(cli_message = function(msg) { …
2. get("pkg_install_make_plan", asNamespace("pak"))(...)
3. prop$stop_for_solution_error()
4. private$plan$stop_for_solve_error()
5. pkgdepends:::pkgplan_stop_for_solve_error(self, private)
6. base::throw(new_error("Could not solve package dependencies:\n", msg, …
7. | base::signalCondition(cond)
8. global (function (e) …
Execution halted
1
infotroph commented 1 year ago

unfortunately install.packages() does not error when it fails to install a package

That's a compelling point and I agree it puts a hard limit on the reliability of remotes's exit statuses. I may dig around more to satisfy my curiosity on where the error went, but will do it with low priority.