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 dependency install when not upgrading #1013

Merged
merged 6 commits into from Jan 20, 2016

Conversation

Projects
None yet
3 participants
@gaborcsardi
Member

gaborcsardi commented Jan 12, 2016

It was broken. Correct me please if I am mistaken.

As I see it, NA means that the package is not installed / available,
and if upgrade=FALSE holds, then we only want to install the
ones that are not installed locally. I.e. the ones with is.na(installed).

(Btw. it seems that devtools ignores version requirements here, but
that is another issue.)

@hadley

This comment has been minimized.

Member

hadley commented Jan 12, 2016

I think that logic holds. @jimhester can you please also check?

@jimhester

This comment has been minimized.

Member

jimhester commented Jan 12, 2016

That looks correct to me, current behavior seems broken.

@gaborcsardi it would be great to have a test for this as well to prevent a regression in the future. Seems a little tricky to test though, you could supply a parse_deps() object to update.package_deps() with a mock install_packages function.

@gaborcsardi

This comment has been minimized.

Member

gaborcsardi commented Jan 12, 2016

Yeah, I am adding a test case in the remotes package, exactly the same way:
https://github.com/MangoTheCat/remotes/blob/800ac4486fba4e3e183559aac6c02ff818d007e8/tests/testthat/test-install-github.R#L263-L295
I can add it here, too.

Need some more time to get it right, though. Might be easier to create an object by hand.

@gaborcsardi

This comment has been minimized.

Member

gaborcsardi commented Jan 12, 2016

Btw. I found this while trying to get 100% test coverage for remotes. This was the single line of code not covered. :)

covr FTW!

@gaborcsardi

This comment has been minimized.

Member

gaborcsardi commented Jan 12, 2016

I'll refactor the whole compare_versions code, because I just can't keep the -2, ..., 2 codes in my mind.... they also seem to be broken (another way), but I am not sure until I refactored it......

The refactoring can be in another PR, or here, as you wish.

EDIT: not sure

@hadley

This comment has been minimized.

Member

hadley commented Jan 12, 2016

Here is fine

@gaborcsardi

This comment has been minimized.

Member

gaborcsardi commented Jan 13, 2016

I have added test cases.

I have also added comments to compare_versions and related functions, and renamed compare_versions arguments, for readability. (Did not find more bugs.)

Rebased.

@jimhester

This comment has been minimized.

Member

jimhester commented Jan 13, 2016

@gaborcsardi maybe we could just set some constants for the various numeric codes, which should make this more self-documenting. https://github.com/gaborcsardi/devtools/pull/1 has an implementation, although I did not use the constants in the tests.

Also I am not sure my choice of UNINSTALLED was the best, maybe NOTINSTALLED would be better?

@jimhester jimhester self-assigned this Jan 13, 2016

@hadley

This comment has been minimized.

Member

hadley commented Jan 13, 2016

@jimhester I like that approach!

@gaborcsardi

This comment has been minimized.

Member

gaborcsardi commented Jan 13, 2016

Merged Jim's PR, and rebased.

@hadley hadley merged commit 8d2c2a6 into r-lib:master Jan 20, 2016

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment