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

feature: Added support for pyproject.toml. #117

Conversation

wouterweerkamp
Copy link

Added support for pyproject.toml, see #116

Installs requirements, fails when not run in FULL mode (not supported for non-setuptools builds).

…ls when not run in FULL mode (not supported for non-setuptools builds).
@MichaelAquilina
Copy link
Owner

Thanks for this @wouterweerkamp. Could you provide some instructions about how I can test this locally?

@MichaelAquilina MichaelAquilina changed the base branch from master to next December 11, 2019 11:02
@MichaelAquilina
Copy link
Owner

Are there any restrictions we need to check for in terms of python version? (I am not very familiar with pyproject.toml)

@wouterweerkamp
Copy link
Author

Thanks for this @wouterweerkamp. Could you provide some instructions about how I can test this locally?

I recently started using poetry (https://poetry.eustace.io/) for Python projects. You could download that and give it a try. When you run poetry new <somename> it'll create a new Python project/folder and in it will be a pyproject.toml file.

Are there any restrictions we need to check for in terms of python version? (I am not very familiar with pyproject.toml)

Not that I'm aware of. When using poetry, the pyproject.toml file will have a Python version in it, but that will be the same one as used to install poetry. Not sure if we need to take that into account.

printf "Found a pyproject.toml file, but editable mode currently requires a setup.py based build.\n"
printf "Set AUTOSWITCH_PIPINSTALL to FULL to disable editable mode.\n"
printf "\n"
return
Copy link
Owner

Choose a reason for hiding this comment

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

this message works but exists too early. It should exit before the .venv file is created to prevent the user's system ending up in a weird state

Copy link
Author

Choose a reason for hiding this comment

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

When AUTOSWITCH_PIPINSTALL is not set to FULL, and when encountering a pyproject.toml file, we have three options when trying to install requirements:

  1. Ignore the AUTOSWITCH_PIPINSTALL setting, and simply run pip install .. Upside: it will work. Downside: it is confusing for users (which could be solved with a message, but still).
  2. Don't run any pip install, but just display the message indicating users should set AUTOSWITCH_PIPINSTALL to FULL. It will create a .venv, but just not install dependencies.
  3. Don't create .venv, don't (try to) install dependencies, only display a message.

Between 2 and 3, I think 2 is easier to implement and with a decent message for users, it should also be understandable to them. Besides, they still have the chance of running pip install . by themselves in the new virtual env.

Copy link
Owner

Choose a reason for hiding this comment

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

I think 1) is actually my preferred option in this case. While it is slightly confusing, it seems like if pyproject.toml does not support the alternative option then we should just use what works without interrupting the user.

Copy link
Author

Choose a reason for hiding this comment

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

I guess you are right. The name of the AUTOSWITCH_PIPINSTALL variable made it sound like something that has influence on various decisions, but it really is only there to distinguish between pip install in editable mode or not for setup.py projects. The name of the variable is a bit confusing perhaps.

Anyway, I'll make the changes as suggested (option 1).

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah I agree, the configuration name is a bit of a misnomer. Might make sense to try migrate to a different name.

Copy link
Owner

@MichaelAquilina MichaelAquilina left a comment

Choose a reason for hiding this comment

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

The change itself looks good. I've test it on my local machine and works as expected apart from a few minor issues.

@rnc
Copy link
Contributor

rnc commented Dec 11, 2019

Is this not related to #49 ? I have recently started using https://github.com/Woile/commitizen.git and needed to work out what poetry needs. From reading python-poetry/poetry#571 running something like

source "$(poetry debug:info | grep Path | awk '{print $3}')/bin/activate"

works for me on latest Fedora 31 (which ships with poetry-0.12.17-3.fc31.noarch).

I wonder if we could integrate that parsing? ( I realise later versions of poetry have poetry env info --path

@wouterweerkamp
Copy link
Author

Is this not related to #49 ? I have recently started using https://github.com/Woile/commitizen.git and needed to work out what poetry needs.

Not entirely. As far as I understand, pyproject.toml is a way to abstract away from only using setuptools to build packages. With a pyproject file, you would just define your build system as dependency, allowing any build system to be used (setuptools, poetry, flit).

Point is, pyproject.toml is not poetry specific, but poetry does use pyproject.toml. My request was purely for supporting pyproject.toml, as a way to recognize that there is a Python project inside a folder (just like checking for requirements.txt and setup.py).

The case you describe is also valid, but different from what I was proposing. I guess in your use case, we would have to look for something Poetry specific, not a pyproject.toml file per se. So, either "look inside" the pyproject file to find "poetry" mentions, and then check if a venv already exists. But again, I think that is a different use case than this one.

@MichaelAquilina MichaelAquilina changed the base branch from next to master December 12, 2019 22:16
@MichaelAquilina MichaelAquilina changed the base branch from master to next December 12, 2019 22:17
Copy link
Owner

@MichaelAquilina MichaelAquilina left a comment

Choose a reason for hiding this comment

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

looks good :)

@MichaelAquilina MichaelAquilina merged commit 42ad049 into MichaelAquilina:next Dec 13, 2019
@MichaelAquilina
Copy link
Owner

This has been merged to next. Once I get a chance to do a bit more testing I'll prepare a release to master.

Thanks for the contribution @wouterweerkamp !

@wouterweerkamp
Copy link
Author

This has been merged to next. Once I get a chance to do a bit more testing I'll prepare a release to master.

Thanks for the contribution @wouterweerkamp !

Cool, thanks for the feedback!

@wouterweerkamp wouterweerkamp deleted the issue-116-support-pyproject branch December 13, 2019 10:47
@MichaelAquilina
Copy link
Owner

@wouterweerkamp an issue I just thought of is that a lot of projects contain a pyproject.toml due to the fact they use for e.g. black settings. However the pyproject.toml would not actually contain the speciifc dependencies. They might use pip/pipenv instead. Right now we prioritize those two before pyproject.toml so there doesnt seem to be much conflict. However we might need to test this for some further corner cases.

@rnc rnc mentioned this pull request Jan 5, 2020
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.

3 participants