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 usethis as Config/Needs/website dependency #1572

Merged
merged 8 commits into from
Mar 19, 2021
Merged

Conversation

maelle
Copy link
Collaborator

@maelle maelle commented Mar 15, 2021

Related to #1552

Copy link
Member

@hadley hadley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you merge, can you please adjust the commit message so that the title is:

Install usethis in pkgdown workflow

And the body is:

Ensures that usethis functions are autolinked. Fixes #1552.

In general I think "fixes #issue-number" should only ever go in the body, not the title.

@maelle
Copy link
Collaborator Author

maelle commented Mar 15, 2021

for some reason the change wasn't enough, no auto-linking yet. 👀

@maelle
Copy link
Collaborator Author

maelle commented Mar 15, 2021

I am still missing some understanding of how to tell pak to install Config/needs/website, the docs of pkgdepends::as_pkg_dependencies() don't help me. 🤔

@maelle
Copy link
Collaborator Author

maelle commented Mar 18, 2021

Maybe I'll wait for actions/toolkit#713 to be fixed (to then exclude pak from the cache)

install.packages("pak", repos = "https://r-lib.github.io/p/pak/dev/")
saveRDS(pak::pkg_deps("local::.", dependencies = TRUE), ".github/r-depends.rds")
shell: Rscript {0}
sudo R -q -e 'install.packages("pak", lib = .Library, repos = "https://r-lib.github.io/p/pak/dev/")'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need the sudo here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was an idea suggested by @gaborcsardi, because without sudo I was told the folder was not writable.
The whole problem in this workflow is that I want to install pak from its daily repo but pak is cached with all other packages and therefore the cached version overwrites the old version.
In the end I might just change the cache name.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bumping the -1- to -2- cleared the cache already, I guess? But this is a general problem with this workflow: if the cache is restored then an older version of pak is potentially restored as well.

We tried to install pak into .Library, so it is not cached. But that apparently needs sudo, and then the cache files are owned by root, which causes different problems.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, I'd suggest eliminating everything from this PR except for the Needs field in the DESCRIPTION and opening an issue in r-lib/actions so we fix this upstream.

@maelle maelle changed the title tweak pkgdown workflow to install usethis fix #1552 Add usethis as Config/Needs/website dependency Mar 19, 2021
@maelle maelle merged commit e409937 into master Mar 19, 2021
@maelle maelle deleted the maelle-patch-1 branch March 19, 2021 12:24
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 this pull request may close these issues.

3 participants