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

install_version() with multiple repositories #305

Merged
merged 13 commits into from Jul 10, 2020

Conversation

kenahoo
Copy link
Contributor

@kenahoo kenahoo commented Feb 28, 2019

This is a port of my previous PR (r-lib/devtools#1259) to the remotes codebase. There was also previous discussion in another PR (r-lib/devtools#1107).

The relevant new features (from the NEWS.md file) are:

* `install_version()` now keeps searching subsequent repositories for the
  requested version, rather than failing if the version it finds in an early
  repository is unsuitable.

* `install_version()` now understands specifications like '>= 1.0' or
  '>= 1.12.0, < 1.14' to install the first version of the package it can
  find that satisfies the criteria.

Along the way, I also created handy functions like the following, but I didn't export them:

  • version_from_tarball, to extract a version string from a filename, e.g. ROI.plugin.glpk_0.0-1.tar.gz -> 0.0-1
  • satisfies, to check whether a given version satisfies a set of criteria, e.g. satisfies('2.0', '< 2.1, > 1.5')
  • have, to check whether a given package is installed and its version matches a set of criteria, e.g. have('dplyr', '< 2.1, > 1.5')
  • version_criteria, which exposes some of the existing parse_deps functionality without needing to specify an actual package

These could be exported if desired.

Thanks.

@kenahoo
Copy link
Contributor Author

@kenahoo kenahoo commented Feb 28, 2019

I don't think the AppVeyor or Travis failures have anything to do with this PR, it seems to be about this:

── 1. Failure: install-github.R script is up to date (@test-script.R#12)  ──────
  readLines(tmp) not equal to readLines(file.path(root, "install-github.R")).
  Lengths differ: 4709 is not 4616

Is that a known failure?

@kenahoo
Copy link
Contributor Author

@kenahoo kenahoo commented Mar 8, 2019

Hi @gaborcsardi & @jimhester, any feedback on this? Thanks!

@kenahoo
Copy link
Contributor Author

@kenahoo kenahoo commented Apr 11, 2019

I'm still very interested in this functionality, it would make our life MUCH MUCH easier for dependency management. Are you interested in incorporating this, or would I be better off trying to put this in a different package?

@kenahoo
Copy link
Contributor Author

@kenahoo kenahoo commented May 8, 2019

I can't really wait longer for this functionality, so I'm creating my own fork of the package under a different name. If you can let me know sometime by commenting on this ticket whether it could be integrated here, I can abandon my fork. Thanks.

Copy link
Member

@jimhester jimhester left a comment

Thank you for your patience and for porting the code!

In general the code needs to follow the tidyverse style guide, in particular - use _ to separate words rather than .

  • always use double quotes for strings
  • conditionals should always use braces, even if a single line

I think the helper function names' are too generic, also it doesn't seem like have() is used anywhere but in the tests?

@kenahoo
Copy link
Contributor Author

@kenahoo kenahoo commented Jun 10, 2019

Thanks @jimhester , I'll make those style adjustments.

Looks like there are some conflicts to resolve too, would you prefer a master->branch merge, or a rebase? I usually like rebases to keep the history clean, but I can do either.

@kenahoo kenahoo force-pushed the install_version-multi2 branch from 0660c8e to 509fb21 Compare Jun 10, 2019
@kenahoo
Copy link
Contributor Author

@kenahoo kenahoo commented Jun 10, 2019

I did some renaming:

  • satisfies() -> version_satisfies_criteria()
  • have() -> package_installed()

I also rebased against master.

You're correct that have() (now package_installed()) isn't used anywhere outside the tests, it's something I used in previous versions of the code. I think it's a handy counterpart to the other similar functions, so I left it in. I think it would be useful e.g. to check whether all requirements in a package's DESCRIPTION file were satisfied. If you weren't interested in it, though, certainly it (and its tests) could be left out.

@kenahoo
Copy link
Contributor Author

@kenahoo kenahoo commented Jun 10, 2019

Sorry @jimhester , I just realized you proposed some concrete changes and I think I blew them away when I rebased.

@kenahoo
Copy link
Contributor Author

@kenahoo kenahoo commented Jun 24, 2019

Hi @jimhester , any feedback?

@kenahoo
Copy link
Contributor Author

@kenahoo kenahoo commented Jul 2, 2019

Hi @jimhester , another reminder to take a look at this again if/when you have a chance. Thanks.

@kenahoo
Copy link
Contributor Author

@kenahoo kenahoo commented Jul 23, 2019

@jimhester @gaborcsardi I would really love it if someone could take a look at this, is there a particular problem I can help address? Thanks.

@kenahoo
Copy link
Contributor Author

@kenahoo kenahoo commented Aug 21, 2019

Hi @jimhester , I noticed you referenced this PR in #426 , I was afraid it had been forgotten. Any chance it might get merged sometime soon? This functionality is still extremely important to us.

@wkdavis
Copy link

@wkdavis wkdavis commented May 20, 2020

I have an issue similar to #495 that would be resolved with this PR. Are there plans to merge in this functionality at some point? It's extremely difficult to install and use this pull request because it has version 2.1.0.9000 but devtools imports remotes >= 2.1.1, causing a conflict.

@kenahoo
Copy link
Contributor Author

@kenahoo kenahoo commented May 20, 2020

@wkdavis I have been trying to get this PR merged for four years (starting with r-lib/devtools#1259, which I ported over to this repo). All the feedback I've gotten was that this change would be desirable, but I can't keep trying to "support" this MR as the codebase changes out from under it as the years pass.

I don't really have any faith that it's ever going to be incorporated, I guess.

@wkdavis
Copy link

@wkdavis wkdavis commented May 21, 2020

@kenahoo totally understandable, thanks for the feedback.

@kenahoo
Copy link
Contributor Author

@kenahoo kenahoo commented Jun 4, 2020

@jimhester can you please remove the "requested changes" tag on this, since I believe those changes have been addressed? Wondering if that's preventing this from getting looked at.

@jimhester jimhester merged commit c53d343 into r-lib:master Jul 10, 2020
0 of 2 checks passed
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

3 participants