Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

avoid warning when missing R cache dir #1129

Merged
merged 2 commits into from
Nov 24, 2023

Conversation

MichaelChirico
Copy link
Contributor

We have a strange setup where tools::R_user_dir() won't be writeable during testing, so normalizePath() throws a warning here. The code works as expected after the warning (cache is just deactivated), so setting mustWork=FALSE here seems reasonable.

OTOH, this might be pretty unique to our testing systems, so if it's too niche an issue we'll just maintain this patch in our fork.

@codecov-commenter
Copy link

codecov-commenter commented Jun 10, 2023

Codecov Report

Merging #1129 (99d64b2) into main (c38d7f9) will not change coverage.
The diff coverage is n/a.

❗ Current head 99d64b2 differs from pull request most recent head 3a13ef6. Consider uploading reports for the commit 3a13ef6 to get more accurate results

@@           Coverage Diff           @@
##             main    #1129   +/-   ##
=======================================
  Coverage   92.35%   92.35%           
=======================================
  Files          46       46           
  Lines        2654     2654           
=======================================
  Hits         2451     2451           
  Misses        203      203           

@github-actions
Copy link
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 8f839e6 is merged into main:

  •   :ballot_box_with_check:cache_applying: 200ms -> 201ms [-1.27%, +2.12%]
  •   :ballot_box_with_check:cache_recording: 723ms -> 720ms [-1.17%, +0.4%]
  •   :ballot_box_with_check:without_cache: 1.35s -> 1.36s [-0.08%, +0.63%]

Further explanation regarding interpretation and methodology can be found in the documentation.

@lorenzwalthert
Copy link
Collaborator

normalizePath() throws a warning here

In my understanding, a warning is given if the path does not exist. Also, in the help file, searching for writ does not give any results.

So is it non-existant or non-writable?

@MichaelChirico
Copy link
Contributor Author

So is it non-existant or non-writable?

I think non-extant because non-writable.

@lorenzwalthert
Copy link
Collaborator

I think non-extant because non-writable.

Really? Or the other way round?

@lorenzwalthert
Copy link
Collaborator

@MichaelChirico happy to help here, can you catch up?

@MichaelChirico
Copy link
Contributor Author

Sure, what else would you like to see in the PR before merge?

Copy link
Contributor

github-actions bot commented Nov 6, 2023

This is how benchmark results would change (along with a 95% confidence interval in relative change) if c38d7f9 is merged into main:

  •   :ballot_box_with_check:cache_applying: 177ms -> 179ms [-0.64%, +2.52%]
  •   :ballot_box_with_check:cache_recording: 567ms -> 569ms [-0.23%, +1.14%]
  •   :ballot_box_with_check:without_cache: 1.05s -> 1.05s [-0.6%, +0.42%]

Further explanation regarding interpretation and methodology can be found in the documentation.

@lorenzwalthert
Copy link
Collaborator

I think non-extant because non-writable.

I don’t understand that. Can you explain?

@MichaelChirico
Copy link
Contributor Author

i.e. the process doesn't have permission to write to that directory, so it can't create the expected file(s).

@lorenzwalthert
Copy link
Collaborator

Ok thanks. @IndrajeetPatil seems link rot check not working anymore. Can you habe a look?

@lorenzwalthert lorenzwalthert merged commit edf399c into r-lib:main Nov 24, 2023
14 of 17 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants