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

Refactor bundlePackages() #670

Merged
merged 14 commits into from
Feb 23, 2023
Merged

Refactor bundlePackages() #670

merged 14 commits into from
Feb 23, 2023

Conversation

hadley
Copy link
Member

@hadley hadley commented Feb 14, 2023

This turns the main strategy "inside out", so that we find all uninstalled packages, then all packages with missing repo, then collect the package metadata into a list.

I've switched the error and warning to use cli and spent a little time polishing the language. But I'm not sure it's that great yet, so I'd love your feedback :)

I also rewrote inferRPackageDependencies() to use a much simpler strategy since there can only be one appMode.

Fixes #659

This turns the main strategy "inside out", so that we find all uninstalled packages, then all packages with missing repo, then collect the package metadata into a list.

I've switched the error and warning to use cli and spent a little time polishing the language. But I'm not sure it's that great yet, so I'd love your feedback :)

I also rewrote `inferRPackageDependencies()` to use a much simpler strategy since there can only be one `appMode`.
Copy link
Contributor

@aronatkins aronatkins left a comment

Choose a reason for hiding this comment

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

Wordsmithing is unending.


# good to go
packages[[name]] <- info
not_installed <- !vapply(deps$Package, is_installed, logical(1))
Copy link
Contributor

Choose a reason for hiding this comment

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

It's going to take me time to grok that things like is_installed are coming from rlang rather than part of this package. All other external references use packagename::XX names.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, our philosophy is that rlang is the sort of missing stdlib, that we've decided that we can't live with out and use so frequently that :: is high friction.

R/bundlePackage.R Outdated Show resolved Hide resolved
R/bundlePackage.R Outdated Show resolved Hide resolved
R/bundlePackage.R Outdated Show resolved Hide resolved
R/bundlePackage.R Outdated Show resolved Hide resolved
R/bundlePackage.R Outdated Show resolved Hide resolved
@toph-allen
Copy link
Contributor

Just a heads up that Tests are failing on R 3.6.3 and R 3.5.3. Not immediately obvious to me from the GH Actions output what's incompatible.

@hadley
Copy link
Member Author

hadley commented Feb 17, 2023

Ugh, that was a dumb and rather hard to track down mistake.

@hadley
Copy link
Member Author

hadley commented Feb 17, 2023

@aronatkins I think this is worth another review. In summary:

  • We better handle my personal motivation: development versions of packages that I've installed locally (Warn if package versions are newer than those on CRAN #659). We do this by only matching CRAN if the locally-installed package name matches a CRAN package name and the local version is not ahead of the CRAN version.
  • This gives me more confidence that we are detecting true failure states, so the new checkBundlePackages() function errors for both uninstalled packages and packages we don't know how to reinstall. If this causes the initial deployment to fail, you'll lose your work defining the destination, but I plan to fix that in saveDeployment() before upload? #677.
  • Since the failure mode is hard to fully explain in an error message, I have fleshed out the docs in appDependencies() and point there from the error.

There is some mild risk that I have introduced bugs that are not caught by the unit tests because this code is hard to test. I think this is somewhat mitigated by the fact that I will be using the dev version of rsconnect, have a mix of CRAN + local + GitHub packages, and will be actively deploying projects over the next few weeks.

@toph-allen
Copy link
Contributor

Ugh, that was a dumb and rather hard to track down mistake.

stringsAsFactors strikes again!

Copy link
Contributor

@aronatkins aronatkins left a comment

Choose a reason for hiding this comment

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

OK, with testing concerns about archived packages.

R/bundle.R Outdated Show resolved Hide resolved
R/bundlePackage.R Outdated Show resolved Hide resolved
R/bundle.R Outdated
# packrat automatically fills in the repository information. But that
# causes problems if you're working with a developement version that's
# newer than CRAN.
repo_version <- package_version(repo.packages[pkg, "Version"])
Copy link
Contributor

Choose a reason for hiding this comment

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

What defines package_version? Could you explain how this will cope with archived packages (e.g. Kmisc, from #508)?

Copy link
Member Author

Choose a reason for hiding this comment

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

package_version() is the base R function that takes a string and turns it into a version object so that 1.2.3 compares correctly to 1.10.4.

Good question about archived packages. I will investigate.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks (on both). I'm more used to seeing packageVersion and forget about package_version.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I think I've fixed it but I need to figure out how to test it and that feels like a post-weekend problem 😄

#Conflicts:
#	NEWS.md
#	R/appDependencies.R
#	R/bundlePackage.R
#	tests/testthat/_snaps/bundlePackage.md
#	tests/testthat/test-bundlePackage.R
And fix capitalisation in docs
@hadley
Copy link
Member Author

hadley commented Feb 23, 2023

I reverted the changes to the findPackageRepoAndSource() to preserve the existing algorithm, so this PR reverts to a pure refactoring. I plan to merge this shortly and then work on some more refactoring before re-tackling the motivating problem.

@hadley hadley merged commit e1f5511 into main Feb 23, 2023
@hadley hadley deleted the bundlePackages-messaging branch February 23, 2023 22:09
hadley added a commit that referenced this pull request Feb 27, 2023
hadley added a commit that referenced this pull request Feb 27, 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.

Warn if package versions are newer than those on CRAN
3 participants