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

sqlalchemy cannot be deployed from Linux #1356

Closed
sproctor opened this issue Feb 17, 2020 · 6 comments
Closed

sqlalchemy cannot be deployed from Linux #1356

sproctor opened this issue Feb 17, 2020 · 6 comments
Labels

Comments

@sproctor
Copy link
Contributor

Packages with uppercase letters in their name, such as SQLAlchemy are not handled properly on case-sensitive file systems. The directory contained in the package has uppercase letters, but the package name is normalized to make them lowercase. Getting rid of the lower() fixes the issue.

def _normalize_name(self, name):
    # type: (str) -> str
    # Taken directly from PEP 503
    return re.sub(r"[-_.]+", "-", name).lower()
@jamesls
Copy link
Member

jamesls commented Feb 25, 2020

Can you clarify what specifically isn't being handled properly? Here's what I tried, which is the behavior I'd expect:

$ cat app.py
import sqlalchemy
from chalice import Chalice

app = Chalice(app_name='foo')


@app.route('/')
def index():
    return {'sqlalchemy': sqlalchemy.__file__}

$ cat requirements.txt
SQLAlchemy

# Note I'm on linux.
$ lsb_release -a
No LSB modules are available.
Distributor ID:	Ubuntu
Description:	Ubuntu 16.04.6 LTS
Release:	16.04
Codename:	xenial


$ chalice deploy
Creating deployment package.
Creating IAM role: foo-dev
Creating lambda function: foo-dev
Creating Rest API
Resources deployed:
  - Lambda ARN: arn:aws:lambda:us-west-2:927523630115:function:foo-dev
  - Rest API URL: https://abcd.execute-api.us-west-2.amazonaws.com/api/

$ curl https://abcd.execute-api.us-west-2.amazonaws.com/api/
{"sqlalchemy":"/var/task/sqlalchemy/__init__.py"}

Are you seeing different behavior?

@marklynch
Copy link

I’ll see if I can put a simple replication together tomorrow to demonstrate what is happening.

@sproctor
Copy link
Contributor Author

I'm out of town right now. I have a similar example that doesn't work for me. I'll test this when I'm home.

@jamesls
Copy link
Member

jamesls commented Feb 26, 2020

Oh interesting. I added an integration test that packages sqlalchemy, and it fails on travis: https://travis-ci.org/aws/chalice/jobs/655456628.

I was testing on an ubuntu box through VMWare on my Mac, but let me spin up an EC2 ubuntu instance and see if I can repro it there.

As far as that line about normalization goes, I'm hesitant to change it. That's taken directly from pep 503 (https://www.python.org/dev/peps/pep-0503/) and it's also what pip uses: https://github.com/pypa/pip/blob/master/src/pip/_vendor/packaging/utils.py#L17

But let me dig into this more and see what I can find.

cc @stealthycoin

@jamesls
Copy link
Member

jamesls commented Feb 26, 2020

Confirmed on an ec2 ubuntu instance.

On reading the myriad of PEPs, it does appear we're doing this wrong:

PEP 426

All comparisons of distribution names MUST be case insensitive, and MUST consider hyphens and underscores to be equivalent.

PEP 508

However, PyPI places strict restrictions on names - they must match a case insensitive regex or they won't be accepted. Accordingly, in this PEP we limit the acceptable values for identifiers to that regex. A full redefinition of name may take place in a future metadata PEP. The regex (run with re.IGNORECASE) is:

So while our normalization is correct, we should also be doing this normalization on directory names as well when checking for a match. I'll get a fix up for this.

@jamesls jamesls added the bug label Feb 26, 2020
jamesls added a commit to jamesls/chalice that referenced this issue Feb 26, 2020
jamesls added a commit to jamesls/chalice that referenced this issue Feb 26, 2020
jamesls added a commit to jamesls/chalice that referenced this issue Feb 26, 2020
@jamesls
Copy link
Member

jamesls commented Feb 26, 2020

Ended up fixing the bug in the linked PR that added an integ test for packaging sqlalchemy. Thanks for reporting!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants