Skip to content

fix(python): dependency versioning in Python #677

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.

Sign up for GitHub

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

Closed
garnaat wants to merge 1 commit into from
Closed

fix(python): dependency versioning in Python #677

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 2 additions & 8 deletions packages/jsii-pacmak/lib/targets/python.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1115,15 +1115,9 @@ class Package {
for (const depName of Object.keys(expectedDeps)) {
const depInfo = expectedDeps[depName];
// We need to figure out what our version range is.
// Basically, if it starts with Zero we want to restrict things to
// ~=X.Y.Z. If it does not start with zero, then we want to do ~=X.Y,>=X.Y.Z.
// If the version is X.Y.Z, we want to allow any compatible version X.Y.n
const versionParts = depInfo.version.split(".");
let versionSpecifier: string;
if (versionParts[0] === "0") {
versionSpecifier = `~=${versionParts.slice(0, 3).join(".")}`;
} else {
versionSpecifier = `~=${versionParts.slice(0, 2).join(".")},>=${versionParts.slice(0, 3).join(".")}`;
}
const versionSpecifier = `~=${versionParts.slice(0, 3).join(".")}`;
Copy link
Copy Markdown
Contributor

@eladb eladb Aug 5, 2019

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?

Copy link
Copy Markdown
Contributor Author

@garnaat garnaat Aug 5, 2019

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?

Copy link
Copy Markdown
Contributor

@eladb eladb Aug 5, 2019

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?

Copy link
Copy Markdown
Contributor Author

@garnaat garnaat Aug 5, 2019

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.

Copy link
Copy Markdown
Contributor

@eladb eladb Aug 5, 2019

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.

Copy link
Copy Markdown
Contributor

@eladb eladb Aug 5, 2019

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 C@1.3.0 it’s requirement on B would be ^1.3.0 which mean that B is not compatible.

Copy link
Copy Markdown
Contributor Author

@garnaat garnaat Aug 5, 2019

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.

Copy link
Copy Markdown
Contributor Author

@garnaat garnaat Aug 6, 2019

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.

Copy link
Copy Markdown
Contributor

@eladb eladb Aug 6, 2019

Choose a reason for hiding this comment

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

Makes sense


dependencies.push(`${depInfo.targets!.python!.distName}${versionSpecifier}`);
}
Expand Down