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

Fix get_extra_deps function #711

Merged
merged 5 commits into from
Feb 4, 2023
Merged

Fix get_extra_deps function #711

merged 5 commits into from
Feb 4, 2023

Conversation

maksymiuks
Copy link
Contributor

Recently I've discovered a bug in the get_extra_deps function. I'll describe it using a reproducible example:

setwd(tempdir())
download.file("https://cran.r-project.org/web/packages/htmlwidgets/DESCRIPTION", "DESCRIPTION")
remotes::dev_package_deps(
  dependencies = c("Depends", "Imports", "LinkingTo", "Suggests", "Enhances")
)

it returns an error saying:

Error: Missing commas separating Remotes: 'shiny (>= 1.1)'

inspection has shown that the problem is in those lines

https://github.com/r-lib/remotes/blob/main/R/deps.R#L212-L213

because standardise_dep(TRUE) returns c("Depends", "Imports", "LinkingTo", "Suggests") which does not necessarily reflects the comments in the code which mentions removing all standard dependencies. "Enhances" although not commonly used is still a standard dependency supported by the R language and developers can specify packages of this type (https://cran.r-project.org/doc/manuals/r-release/R-exts.html 1.1.3 Package Dependencies). Because the current implementation does not include Enhances, this field is interpreted as remotes source, and thus any version specification results in an error. An important note is that although Enhances versions are not used now, R manual clearly states they can be specified.

The PR is a fix to the mentioned issues. It ensures Enahnces packages are recognized as standard dependencies.

@maksymiuks
Copy link
Contributor Author

@gaborcsardi Requesting review

@maksymiuks
Copy link
Contributor Author

@gaborcsardi any thoughts about it?

@maksymiuks
Copy link
Contributor Author

Hi

I see the PR got stale for a while. I'd like to ask what should be the next steps. Should I update the PR so it can get reviewed, or create an issue so we can discuss thoroughly why is it an issue? Would appreciate some guidance @gaborcsardi as the bug is a pain in the neck, every time I have to reinstall remotes from the branch.

@gaborcsardi
Copy link
Member

gaborcsardi commented Feb 4, 2023

Thanks and sorry for the long wait!

@gaborcsardi gaborcsardi merged commit 88fdc4e into r-lib:main Feb 4, 2023
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.

None yet

2 participants