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

[WIP] Ignoring pre-release status when checking against resolved packages #2474

Merged
merged 4 commits into from Jun 30, 2017

Conversation

Projects
None yet
4 participants
@zzdavid

zzdavid commented Jun 30, 2017

Ignoring pre-release status of a new requirement when checking it against packages that have already been resolved. #2471

The thinking is that if something dragged in a pre-relase version (e.g. the dependencies file) then we actually want pre-release of that package.

In my testing this seems to work. Even if the pre-release status is ignored in IsInRange(), the actual pre-release versions are still compared, so a requirement ">=2.0" will fail if a resolved package has version "2.0-pre". Could use some checking from someone who knows the code better though.

David Smith
Ignoring pre-release status of a requirement when checking against re…
…solved versions.

The thinking is that if something dragged in a pre-relase version (e.g. the dependencies file) then we actually want it. Add tests to verify the behaviour of IsInRange(). #2471

@matthid matthid changed the title from Ignoring pre-release status when checking against resolved packages to [WIP] Ignoring pre-release status when checking against resolved packages Jun 30, 2017

@matthid

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Jun 30, 2017

Member

I want to add some test-cases to this before we merge it :)

  • The one from #2471
  • At least one where we ensure that we don't pull prereleases from unrelated space of the search tree
  • At least one where we have the same conflict on transitive packages
  • Maybe more.

We can add these to the resolver unit tests...

See #2558

Member

matthid commented Jun 30, 2017

I want to add some test-cases to this before we merge it :)

  • The one from #2471
  • At least one where we ensure that we don't pull prereleases from unrelated space of the search tree
  • At least one where we have the same conflict on transitive packages
  • Maybe more.

We can add these to the resolver unit tests...

See #2558

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Jun 30, 2017

Member

@zzdavid I think it's not 100% correct. I pushed to your fork (github allows that) to extend your PR. see 08dec38

Member

forki commented Jun 30, 2017

@zzdavid I think it's not 100% correct. I pushed to your fork (github allows that) to extend your PR. see 08dec38

Stift and others added some commits Jun 30, 2017

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Jun 30, 2017

Member

pushed @Stift's tests

Member

forki commented Jun 30, 2017

pushed @Stift's tests

@zzdavid

This comment has been minimized.

Show comment
Hide comment
@zzdavid

zzdavid Jun 30, 2017

@forki if I read the change correctly, you only allow pre-release on packages in the dependencies file? Might that not be too limiting? I was actually happy to allow pre-release unconditionally here since it has already been resolved. We're only checking if the new requirement is ok here, so the proper thing to do is to trust that the resolution is correct and check the version as it is.

Just want to raise the question. If you say this is the way it should be, I'll take your word for it :)

zzdavid commented Jun 30, 2017

@forki if I read the change correctly, you only allow pre-release on packages in the dependencies file? Might that not be too limiting? I was actually happy to allow pre-release unconditionally here since it has already been resolved. We're only checking if the new requirement is ok here, so the proper thing to do is to trust that the resolution is correct and check the version as it is.

Just want to raise the question. If you say this is the way it should be, I'll take your word for it :)

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Jun 30, 2017

Member

@zzdavid yes it's actually needed to restrict it. Otherwise you will end up with following situation:

depsfile:

nuget A

resolution goes:

A 1.1 (deps: B >= 0 prerelease)

A 1.1 
B 1.1-alpha (deps: C >= 0 prerelese)

A 1.1 
B 1.1-alpha 
C 1.2 (deps: B >= 0)

^^ this would be valid with your change, but I think it should not

Member

forki commented Jun 30, 2017

@zzdavid yes it's actually needed to restrict it. Otherwise you will end up with following situation:

depsfile:

nuget A

resolution goes:

A 1.1 (deps: B >= 0 prerelease)

A 1.1 
B 1.1-alpha (deps: C >= 0 prerelese)

A 1.1 
B 1.1-alpha 
C 1.2 (deps: B >= 0)

^^ this would be valid with your change, but I think it should not

@forki forki merged commit 1276e44 into fsprojects:master Jun 30, 2017

1 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@zzdavid

This comment has been minimized.

Show comment
Hide comment
@zzdavid

zzdavid Jun 30, 2017

@forki It looks like a bug in package A (without prerelease) if it depends on a prerelase of B. With package A in pre-relase it makes sense.

nuget A alpha

with the resolution

A 1.1-alpha (deps: B >= 0 prerelease)
B 1.1-alpha
C 1.2 (deps: B >= 0)

The resolution looks correct to me, at least if A and B are developed together, and worked in 4.8.8 but will fail now.

zzdavid commented Jun 30, 2017

@forki It looks like a bug in package A (without prerelease) if it depends on a prerelase of B. With package A in pre-relase it makes sense.

nuget A alpha

with the resolution

A 1.1-alpha (deps: B >= 0 prerelease)
B 1.1-alpha
C 1.2 (deps: B >= 0)

The resolution looks correct to me, at least if A and B are developed together, and worked in 4.8.8 but will fail now.

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Jun 30, 2017

Member

Ok let's say D introduces the B >= 0 restriction. Doesn't matter - point is something later can still need it

Member

forki commented Jun 30, 2017

Ok let's say D introduces the B >= 0 restriction. Doesn't matter - point is something later can still need it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment