-
Notifications
You must be signed in to change notification settings - Fork 246
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(python): dependency versioning in Python #677
Conversation
…requires section of setup.py. Fixes aws#676.
} else { | ||
versionSpecifier = `~=${versionParts.slice(0, 2).join(".")},>=${versionParts.slice(0, 3).join(".")}`; | ||
} | ||
const versionSpecifier = `~=${versionParts.slice(0, 3).join(".")}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to make sure, ~=1.4.0
will be compatible with 1.6.5
, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not totally sure I understand the question. If you use a version string of "~=1.4.0" for a dependency, it will be compatible with 1.4.*. If the only version it can find is 1.6.5, it will fail. Is that not correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In JavaScript the version spec we use is “^1.3.0” which means to use the latest compatible version. This basically includes all versions >= 1.3.0 and < 2.0.0 since by definition those should be backwards compatible with 1.3.0. We should use the same spec for python I think, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that was the problem we were trying to fix in aws/aws-cdk#3517. If I try to install version 1.0.0 of a module and it has dependencies, all of the dependencies come in as the latest available version (e.g. 1.3.0) rather than also being pinned to 1.0.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as 1.3.0 is compatible with 1.0.0, I don’t see a problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmmm... looking at the CDK issue, I am curious how this can happen because basically there is a violation of the requirement for the transitive dependency. If there’s a top level dependency A-1.0.0 and B-1.0.0 and day A has a transitive dependency on C, which takes a dependency on B. If we bring in [email protected] it’s requirement on B would be ^1.3.0 which mean that B is not compatible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, from the issue above it appears that it is causing a problem. Plus, I guess if I pinned my dependency to a specific version my expectation is that all transitive dependencies would be pinned to the same version. If we don't think that is the right behavior this fix is not needed as there really isn't a problem to fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this PR should be merged. It would mean that Python would work differently than the other languages. Instead, I will investigate the linked issue and try to reproduce it and figure out why it caused an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense
Closing this PR without merging. We can dig it up if we end up actually needing to go down that path. |
@eladb @RomainMuller this not being merged causes aws/aws-cdk#4957 to cascade into any of my libraries even if I pin them at 1.15.0 because is making an incorrect compatible release specification (https://www.python.org/dev/peps/pep-0440/#compatible-release)
|
I think the reasoning is slightly different now however - the code generator generates a dependency statement that is NOT what was declared in the initial module! |
@RomainMuller I can understand that concern Regarding the python dependency itself keying off depInfo.version and exclusively using the |
@RomainMuller just to be thorough. There is a explicit difference in the interpretation of versions that makes an incorrect compatibility presumption with regard to the current python implementation where ~=X.Y,>=X.Y.Z is being returned for the python version. In npmjs tilde ranges the same In python compatible release The change in this PR where the version is set to be exclusively as I wasn't sure if this difference was clear or not so please forgive me posting twice :) |
So at risk of being completely redundant if the code was updated such that the python version uses depInfo.version the string still needs to be updated to be exclusively |
Fix the generation of the dependency version strings for the install_requires section of setup.py. Fixes #676.
The version string placed in the setup.py file for dependencies is incorrect. This results in incompatible modules being installed. This fix changes the generated version string for dependencies to correct the problem.
Fixes #696
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.