Add support for poetry. #308
Conversation
appveyor.yml
Outdated
| @@ -1,6 +1,7 @@ | |||
| version: '{build}' | |||
| init: | |||
| - cmd: pip install pipenv | |||
| - cmd: curl https://raw.githubusercontent.com/sdispater/poetry/master/get-poetry.py -o get-poetry.py && python get-poetry.py --preview --yes && rm get-poetry.py | |||
There was a problem hiding this comment.
Any particular reason for installing the prerelease?
There was a problem hiding this comment.
The export command is currently only in the prerelease version.
There was a problem hiding this comment.
That's a great reason. Could you also document that in the readme as to avoid bug reports from users of the stable version of poetry.
lib/poetry.js
Outdated
|
|
||
| this.serverless.cli.log('Generating requirements.txt from pyproject.toml...'); | ||
|
|
||
| const res = spawnSync('poetry', ['export', '-f', 'requirements.txt'], { |
There was a problem hiding this comment.
Any reason not to export 'export', '-f', path.join(this.servicePath, '.serverless', 'requirements.txt') directly and avoid having to move it later?
There was a problem hiding this comment.
-f stands for format not file. The only format supported right now is requirements.txt which places the exported requirements.txt in the current directory.
tests/poetry/unzip_requirements.py
Outdated
| @@ -0,0 +1,20 @@ | |||
| import os | |||
There was a problem hiding this comment.
Remove this file and add a .gitignore in the tests/poetry dir to avoid checking it in since it's generated by the test process
| function pyprojectTomlToRequirements() { | ||
| if ( | ||
| !this.options.usePoetry || | ||
| !fse.existsSync(path.join(this.servicePath, 'pyproject.toml')) |
There was a problem hiding this comment.
one more question. Should this check the contents of the pyproject.toml file since other tools are configured with it too?
There was a problem hiding this comment.
Hmm. Do you know what other tools might be configured to use it as well? I thought that file was introduced by poetry.
I did add support to disable this functionality so if someone is using a pyproject.toml without poetry, then they can tell this plugin to ignore the file.
If I were to check the contents, that would most likely add another dep to this plugin so I can parse the toml. Is that something you really want me to do?
There was a problem hiding this comment.
Do you know what other tools might be configured to use it as well?
The only tool I know of currently is flit, but I don't think that's for installation, only publication.
I thought that file was introduced by poetry.
Not sure which introduced it, but it's specified in PEP518 and there's nothing stopping someone from making a new packaging tool that uses it
If I were to check the contents, that would most likely add another dep to this plugin so I can parse the toml. Is that something you really want me to do?
fair point. Probably not. Lets leave it like this for now and we can revisit if another package manager leveraging pep518 pops up
|
@dschep this is nearly ready for another review, but I'm struggling to get the windows tests to pass as I'm not familiar with appveyor or windows development. Please excuse all the force pushes to this branch for now as I'm working out how to get the windows system to see the poetry install for tests. |
Why: * [poetry](https://github.com/sdispater/poetry) is a Python dependency management and packaging tool. This change addresses the need by: * Taking the same approach as pipenv. * Install poetry on CircleCI and AppVeyor for tests. * Use chmodSync in test.js as it works better cross platform for setting exact permissions on files. See: nodejs/node#1104 Side effects: * There may be conflicts or oddities if there is a Pipfile and a pyproject.toml.
|
@dschep sorry for the noise today with all the force pushes. I figured out a way to test on windows by installing poetry with pip which is not the ideal method as it then shares deps with other pip installed packages, but tests now pass across the board. This PR is now ready for another review and potential merge. Thank you for all your feedback. |
|
Oh man! #310 looks awesome 🎉 |
|
Hmm, @squaresurf looks like |
|
Interesting. Should we add a toml parser to this so we can read the purpose of the pyproject.toml? We may even want to use the pyproject.toml for pip as well in the future. |
|
Yup, I think it's reasonable to add a toml parser 👍 |
|
Do you have any recommendations? I'm not super deep in the node ecosystem. |
|
not really. https://www.npmjs.com/package/@iarna/toml and https://www.npmjs.com/package/toml seem to be the most used, but the later only supports toml 0.4, not sure what pep517 specifies |
|
Hello, I would like to know if it's possible to disable this behavior? we use poetry for our local development but we export a requirement.txt from it. Your CI/CD machines don't have poetry installed on them and we are using a forked version of poetry anyway, so installing it a bit tricky. EDIT: Nevermind my previous comment, I just found the But I double checked and I just noticed that we are not even using Poetry at all for this project, but we do have a pyproject.toml file in order to configure Black. Maybe serverless-python-requirements is assuming that if a project has a pyproject.toml file it, therefore, is using Poetry? |
|
@dschep heads up that I may not be able to get to this for a while. |
|
Ok. Thanks for letting me know @squaresurf |
Why:
management and packaging tool.
This change addresses the need by:
Side effects:
pyproject.toml.