Overwrite files if they exist #84
Conversation
|
Do you want me to add a note to NEWS or anything? |
|
yes, please. |
The default behavior of `file.copy()` is _not_ to overwrite files (without an error or warning) if they already exist. So if there was already a built tarball in the check directory it would not be updated. cc @jennybc
jimhester
force-pushed
the
file.copy-overwrite
branch
from
bbaae61 to
21f9732
Compare
Codecov Report
@@ Coverage Diff @@
## master #84 +/- ##
==========================================
- Coverage 79.83% 79.78% -0.06%
==========================================
Files 17 17
Lines 749 752 +3
==========================================
+ Hits 598 600 +2
- Misses 151 152 +1
Continue to review full report at Codecov.
|
|
Ok, I think this is good to go then! |
|
This affects attempts to use and if an old tarball exists from a prior build attempt, that old tarball will be used. In effect, a user can get into this state if they run Check inside RStudio while the package has @gaborcsardi after this fix comes in, would you be willing to push a patch-fix release to CRAN? |
|
@gaborcsardi sure, np. |
|
So, one issue with this is that it potentially leaves some old files behind. I think if we are already overwriting, we should |
|
Yeah that is true, we should probably clean out the check directory first. I actually tried that first, because that was what I thought the problem was initially, I will resurrect the code (1e70510) |
R/package.R
Outdated
| @@ -117,6 +117,12 @@ do_check <- function(targz, package, args, libpath, repos, | |||
| profile <- make_fake_profile(session_output = session_output) | |||
| on.exit(unlink(profile), add = TRUE) | |||
|
|
|||
| # if the pkg.Rcheck directory already exists, unlink it | |||
| check_dir <- paste0(package, ".Rcheck") | |||
| if (file.exists(check_dir)) { | |||
There was a problem hiding this comment.
I think we don't need to check for this, just unlink, it will do nothing if it does not exist, anyway. IMO checking for the existence of a file, is almost always an anti-pattern.
There was a problem hiding this comment.
Sure that is fair, race conditions.
There was a problem hiding this comment.
Yeah, it might even make sense to filelock::filelock() the check dir, in case you call multiple checks from multiple sessions, and unlock() in on.exit. But we don't have to do that in this PR.
| @@ -1,6 +1,9 @@ | |||
|
|
|||
| # devel | |||
|
|
|||
| * `rcmdcheck()` now correctly overwrites existing tarballs if they already | |||
| exist in the check directory (#84 @jimhester). | |||
There was a problem hiding this comment.
This is not just the tarballs, right? But overwrites (well, replaces) the whole check directory, right?
There was a problem hiding this comment.
No it is just the tarballs, if it is a directory it uses the other part of the conditional that is unchanged.
| @@ -1,6 +1,9 @@ | |||
|
|
|||
| # devel | |||
|
|
|||
| * `rcmdcheck()` now correctly overwrites existing tarballs if they already | |||
| exist in the check directory (#84 @jimhester). | |||
The default behavior of
file.copy()is not to overwrite files(without an error or warning) if they already exist. So if there was
already a built tarball in the check directory it would not be updated.
cc @jennybc