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

Support PEP 600 platform tags #234

Merged
merged 8 commits into from
May 20, 2021

Conversation

hawflau
Copy link
Contributor

@hawflau hawflau commented May 15, 2021

Issue #, if available:
#233
aws/aws-sam-cli#2865
aws/aws-sam-cli#2882
aws/aws-sam-cli#2888

Description of changes:
Add support for PEP 600 tags (shamelessly copy most code changes from aws/chalice#1731)

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

aahung
aahung previously approved these changes May 17, 2021
Copy link
Contributor

@aahung aahung left a comment

Choose a reason for hiding this comment

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

Interesting, I have no idea this file looks so similar to chalice's.

Thanks for fixing it. Since chalice already merges their PR, I assume it is the right path. Do we need to add credit inside the code or something?

@@ -65,6 +65,7 @@ for:
- "choco install dep"
- setx PATH "C:\go\bin;C:\gopath\bin;C:\Program Files (x86)\Bazaar\;C:\Program Files\Mercurial;%PATH%;"
- "go version"
- "go env -w GO111MODULE=auto"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this trying to solve the same problem as this? aws/aws-sam-cli#2646
If so maybe we should keep them consistent

Copy link
Contributor Author

@hawflau hawflau May 17, 2021

Choose a reason for hiding this comment

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

It seems not the same problem. However, we might need to update the go dep integration test cases to be future-proof of future go version (>=1.17).
https://blog.golang.org/go116-module-changes

@mgrandis
Copy link
Contributor

Could you also add an integration test in tests/integration/workflows/python_pip?

@hawflau
Copy link
Contributor Author

hawflau commented May 18, 2021

Could you also add an integration test in tests/integration/workflows/python_pip?

Added numpy==1.20.3 for Python version >= 3.7. 1.20.3 is the numpy version that starts to use PEP 600 tags and compressed tags.

aahung
aahung previously approved these changes May 19, 2021
Comment on lines +159 to +165
# Mapping of abi to glibc version in Lambda runtime.
_RUNTIME_GLIBC = {
"cp27mu": (2, 17),
"cp36m": (2, 17),
"cp37m": (2, 17),
"cp38": (2, 26),
}
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add a reference here?

_MANYLINUX_LEGACY_MAP = {
"manylinux1_x86_64": "manylinux_2_5_x86_64",
"manylinux2010_x86_64": "manylinux_2_12_x86_64",
"manylinux2014_x86_64": "manylinux_2_17_x86_64",
Copy link
Contributor

Choose a reason for hiding this comment

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

I examined the PEP 600 and manylinux2014_x86_64 is not in there.

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

manylinux2014 is defined in PEP 599

pip.packages_to_download(
expected_args=["-r", requirements_file, "--dest", mock.ANY, "--exists-action", "i"],
packages=[
"foo-1.2-cp36-cp36m-manylinux_2_5_x86_64.manylinux1_x86_64.whl",
Copy link
Contributor

Choose a reason for hiding this comment

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

each tag in a filename can instead be a '.'-separated, sorted, set of tags

Can you double check whether this is sorted?

  1. manylinux_2_5_x86_64
  2. manylinux1_x86_64

Copy link
Contributor

Choose a reason for hiding this comment

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

(and manylinux1_x86_64 is an alias of manylinux_2_5_x86_64, they are the same right)

hoffa
hoffa previously approved these changes May 20, 2021
@hoffa hoffa dismissed stale reviews from aahung and themself via 5840438 May 20, 2021 19:41
@hawflau hawflau merged commit 182ff82 into aws:develop May 20, 2021
aahung pushed a commit to aahung/aws-lambda-builders that referenced this pull request May 21, 2021
* Support PEP 600 platform tags

* fix test

* Fix integration test in appveyor by setting GO111MODULE to auto

* Fix GOPATH env var in appveyor script

* Add note about setting GO111MODULE in appveyor script

* Update python_pip integration test

* Fix numpy version for py36 in integ test

* Update aws_lambda_builders/workflows/python_pip/packager.py

Co-authored-by: Chris Rehn <crehn@outlook.com>
aahung pushed a commit that referenced this pull request May 21, 2021
* Support PEP 600 platform tags

* fix test

* Fix integration test in appveyor by setting GO111MODULE to auto

* Fix GOPATH env var in appveyor script

* Add note about setting GO111MODULE in appveyor script

* Update python_pip integration test

* Fix numpy version for py36 in integ test

* Update aws_lambda_builders/workflows/python_pip/packager.py

Co-authored-by: Chris Rehn <crehn@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants