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

fix: get pipenv py version from Pipfile #7254

Merged
merged 1 commit into from
May 6, 2021

Conversation

edwardfoyle
Copy link
Contributor

Description of changes

When resolving the virtual env site packages file, the path was constructed using the system python version rather than the virtual env python version. This change constructs the path by parsing the PIpenv file and using the python version found there

Issue #, if available

#7188

Description of how you validated changes

Manually tested. Existing E2E tests cover creating building and packaging python functions

Checklist

  • PR description included
  • yarn test passes

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@edwardfoyle edwardfoyle requested a review from a team as a code owner May 4, 2021 20:05
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM with a question.

@@ -13,8 +14,7 @@ export async function getPipenvDir(srcRoot: string): Promise<string> {
throw new Error(`Could not find 'python3' or 'python' executable in the PATH.`);
}

const pyVersion = await execAsStringPromise(`${pyBinary} --version`);
let pipEnvPath = path.join(pipEnvDir, 'lib', 'python' + majMinPyVersion(pyVersion), 'site-packages');
let pipEnvPath = path.join(pipEnvDir, 'lib', `python${getPipfilePyVersion(path.join(srcRoot, 'Pipfile'))}`, 'site-packages');
Copy link
Contributor

Choose a reason for hiding this comment

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

Would there be any benefit to falling back to majMinPyVersion() if getPipfilePyVersion() fails?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If there's no version specified in the Pipfile, creating the virtual env wont even work so I don't think we need a fallback here

@attilah attilah linked an issue May 5, 2021 that may be closed by this pull request
4 tasks
@cjihrig cjihrig merged commit 866f261 into aws-amplify:master May 6, 2021
@cjihrig cjihrig self-assigned this May 6, 2021
@github-actions
Copy link

👋 Hi, this pull request was referenced in the v4.51.0 release!

Check out the release notes here https://github.com/aws-amplify/amplify-cli/releases/tag/v4.51.0.

@github-actions github-actions bot added the referenced-in-release Issues referenced in a published release changelog label May 14, 2021
@edwardfoyle edwardfoyle mentioned this pull request Jun 4, 2021
4 tasks
cjihrig pushed a commit to ctjlewis/amplify-cli that referenced this pull request Jul 12, 2021
When resolving the virtual env site packages file, the path was constructed using the system python version rather than the virtual env python version. This change constructs the path by parsing the PIpenv file and using the python version found there
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
referenced-in-release Issues referenced in a published release changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Python version conflict after initial push command
2 participants