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

setup.py generated by poetry build does not have included file in package_data #1338

Closed
2 tasks done
bdoms opened this issue Aug 28, 2019 · 29 comments
Closed
2 tasks done
Labels
kind/bug Something isn't working as expected

Comments

@bdoms
Copy link

bdoms commented Aug 28, 2019

  • I am on the latest Poetry version.

  • I have searched the issues of this repo and believe that this is not a duplicate.

  • OS version and name: Ubuntu 19.04

  • Poetry version: 0.12.17

Issue

I have a web project that builds a static JS file, which I have explicitly ignored in my .gitignore file, but that I do want to include in builds of my project. Adding that file to the include directive in my pyproject.toml works correctly in that the file is included in the sdist tar, HOWEVER, it's missing from the package_data entry in the generated setup.py, so when I install the package, the file is not included and the installed package is broken.

Here's the setup. I have two files in a package sub-directory:

web/static
|-- app.js
|-- utils.js

Then in .gitignore this line:

web/static/app.js

In pyproject.toml:

packages = [
    {include = "web"}
]

include = ["web/static/app.js"]

Running poetry build -f sdist succeeds. But then setup.py looks like this:

packages = \
['web']

package_data = \
{'': ['*'],
 'web': ['static/utils.js']}

I would expect that a file specified in include would have an entry there, or be covered by another rule, but that's not the case and it breaks things for me.

Let me know if you need more info or how I can help. Poetry is awesome.

@bdoms
Copy link
Author

bdoms commented Sep 6, 2019

Wanted to throw out a potential fix. I don't have time to test this myself right now, but I believe that putting something in the builder around https://github.com/sdispater/poetry/blob/master/poetry/masonry/builders/builder.py#L63 like this would correct the issue:

for included_glob in self._package.include:
    for included in glob(
        os.path.join(self._path.as_posix(), str(included_glob)), recursive=True
    ):
        vcs_ignored_files.discard(
            Path(included).relative_to(self._path).as_posix()
        )

@brycedrennan brycedrennan added the kind/bug Something isn't working as expected label Sep 6, 2019
@con-f-use
Copy link

see: pypa/setuptools#1650

@funkyfuture
Copy link
Contributor

i hit the same when i try to include a marker according to PEP561:

[tool.poetry]
#
include = ["mypackage/py.typed"]
#

which results in

# …
package_data = \
{'': ['*']}
# …

but the file is not excluded in any vcs-related context.

@stale
Copy link

stale bot commented Nov 13, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Nov 13, 2019
@bdoms
Copy link
Author

bdoms commented Nov 13, 2019

Thanks for the warning stale bot. I just tried this again after a poetry self:update and it is definitely still happening, and still an issue if poetry wants to support sdist builds.

@stale stale bot removed the stale label Nov 13, 2019
@sdispater
Copy link
Member

@bdoms Can you try with the latest beta version of the 1.0.0 release?

@gijzelaerr
Copy link

This seems to work correctly for version 1.0.0b4 for me.

@bdoms
Copy link
Author

bdoms commented Nov 15, 2019

@sdispater thanks for checking in! I upgraded to 1.0.0b5 and this is still a problem: a file I had excluded in .gitignore but explicitly included in pyproject.toml is there in the tar but not in setup.py.

The code has moved around a bit since I last checked, but looking at it now the problem appears to be this line: https://github.com/sdispater/poetry/blob/master/poetry/masonry/builders/sdist.py#L137

It's the else case for the if isinstance(include, PackageInclude) check when creating the list of files that becomes the package_data. I have no idea why the files I need fail that check, but all of the files I've been having problems with fail there, so they hit the pass line, and thus are not included in the list.

@ErikBjare
Copy link

ErikBjare commented Jan 7, 2020

I'm not sure how related my problem is to this issue and if it warrants a separate issue, but include = ['package/file.py'] doesn't seem to override my .gitignore as I expected it to. It does not end up in my installed package in site-packages when I run pip install ..

.gitignore:

package/file.py

pyproject.toml:

[tool.poetry]
include = ["package/file.py"]
...
$ pip install .
(installs successfully)
$ ls .../site-packages/package/
...
(no file.py)

Removing the entry from .gitignore works fine, however.

Running poetry build seems to correctly include the file in the wheel and sdist (and doesn't if I don't include the tool.poetry.include entry, as expected).

I'm working on it in ActivityWatch/aw-qt#51

Edit: I made a dirty workaround that temporarily comments out the file from .gitignore before building (and uncomments when build is done), but would appreciate a clean fix.

@fcomabella
Copy link

i hit the same when i try to include a marker according to PEP561:

[tool.poetry]
#
include = ["mypackage/py.typed"]
#

which results in

# …
package_data = \
{'': ['*']}
# …

but the file is not excluded in any vcs-related context.

I'm having the same issue.

@yanyang729
Copy link

I am on Poetry version 1.0.0b4 and having the exact same issue

@KaiserKarel
Copy link

Trying to include some API definitions like so

include = ["../api/*.capnp"]

Am I having problems due to this issue; or are relative paths outside the pyproject.toml directory not supported?

@long2ice
Copy link

same here with version 1.0.9

@abn
Copy link
Member

abn commented Jul 13, 2020

Is this still an issue with the current preview release 1.1.0a3? There has been significant changes in how the includes are handled.

Going forward poetry will by default stop generating setup.py files in the sdists. For now this is now available as an opt-in option via python-poetry/poetry-core#26. However, note that there is a bug in the implementation that will be resolved once python-poetry/poetry-core#43 is merged. In 1.1 this will be opt-in, and in 1.2 this will become default behaviour.

@sebix
Copy link

sebix commented Jul 14, 2020

Going forward poetry will by default stop generating setup.py files in the sdists. For now this is now available as an opt-in option via python-poetry/poetry-core#26.

How can sdists be built and installed then?

@abn
Copy link
Member

abn commented Jul 14, 2020

How can sdists be built and installed then?

You do not necessarily need a setup.py in your sdist thanks to PEP 517. The sdist will still contain the pyproject.toml anyway. Your build frontend (pip etc.) should still be able install from an sdist generated by poetry. Another thing to note here is that once htis becomes default we will still allow for legacy behaviour via the same flag until the setupfile generation is removed altogether - but that is a couple of minor versions away after the next one.

@bdoms
Copy link
Author

bdoms commented Jul 14, 2020

I like Poetry a lot because it has generally made my life easier, but removing setup.py from sdist builds is a backwards incompatible change. There's no guarantee that people are using a version of pip that works without it, or even pip or any other build tool at all - they may just expect setup.py to be there and call it directly. If you're following semantic versioning the backwards incompatibility requires a major version change in order to be implemented. I get that you may not use it yourself, but make no mistake: THIS WILL BREAK PEOPLE'S WORKFLOWS. There's gonna be some bad days when build/deploy systems suddenly stop working and devs are scrambling to find out why.

As for whether it's still happening, I just ran poetry self update on Ubuntu 20.04 with Python 3.8 and the Poetry version I got was 1.0.9. In that version, yes, this is still happening just as described in the initial report above.

@ErikBjare
Copy link

@bdoms We already know it's in 1.0.9 (#1338 (comment)).

FWIW, my particular variant of the issue (as described in #1338 (comment)) appears to have been resolved in 1.1.0a3.

@atollk
Copy link

atollk commented Jul 18, 2020

Still present in 1.1.0a3. Although, I don't even get how the supposed change by @abn should work, as a setup.py is generated even if I include

[tool.poetry.build]
generate-setup-file = false

@funkyfuture @fcomabella Have you found a workaround to use PEP 561 with poetry?

@abn
Copy link
Member

abn commented Jul 18, 2020

removing setup.py from sdist builds is a backwards incompatible change.

@bdoms Yes, we understand that in rare cases where the project maintainer wants to remain compatible with downstream build scenarios that rely on older build toolchain, this might require user intervention. However, supporting a feature like this in perpetuity is not a good idea for a few reason. Maintaining setup.py generation as we expand poetry's capabilities becomes harder as it requires us to take into consideration various cases. This overhead does not make sense in comparison with the small cross section of scenarios that may be invalidated (ie. package manager, pip or otherwise, uses a version that does not understand PEP-517 and only knows setuptools, the platform does not have pre-built wheels for the packages and requires the use of the sdist) and the fact that these issues can easily be worked around. Also it should be noted that the generation of setup.py file was always meant to be a workaround rather than a supported feature. The current implementation of this is in no way considered reliable in all scenarios. Additionally, with poetry>=1.1.0, if the build happens in a poetry managed virtual environment, the pip version used will be the version bundled with virtualenv project, which at the moment is 20.x.

All this said, it is also important to note that this is not happening overnight. The following steps need to happen prior to this being removed.

  1. tool.poetry.build.generate-setup-file introduced in a GA release (1.1.0), defaulting to true. (opt-in)
  2. tool.poetry.build.generate-setup-file defaults to false (1.2.0). (opt-out)
  3. setup.py generation deprecation (unplanned).
  4. setup.py generation removal (unplanned).

@abn
Copy link
Member

abn commented Jul 18, 2020

Still present in 1.1.0a3. Although, I don't even get how the supposed change by @abn should work

@atollk As I mentioned in my original comment (apparently unclearly), the current configuration handling has a bug, it does the inverse of the configuration, which is resolved with python-poetry/poetry-core#26 but that change has not yet made it into a poetry preview release yet. In the interim, try generate-setup-file = true - that should disable the file generation until the fixed version of poetry-core is used. You might also want to update your pyproject.toml to use poetry-core as your build backend to use the recent improvements in the include file logic.

[build-system]
requires = ["poetry-core>=1.0.0a3"]
build-backend = "poetry.core.masonry.api"

Specific to the reported issue, if this is still an issue and the generated setup.py fix is required, this will most likely have to happen in python-poetry/poetry-core, contributions welcome. You can start with adding a test case to try reproduce the issue here,

@atollk
Copy link

atollk commented Jul 18, 2020

@abn I see. I added a PR which should fix the issue. The code in question is what was described above: #1338 (comment) .

@atollk
Copy link

atollk commented Jul 18, 2020

@abn Regarding the point of deprecating setup.py altogether, is there or has there been a longer discussion about that? Afaik, setup.py is still the canonical way of defining and building packages, so I'd be interested to see how your alternative is planned to work. The issue that brought me here, for example, which is adding PEP 561 py.typed to a poetry package, is not possible without a setup.py file, at least to my knowledge.

@abn
Copy link
Member

abn commented Jul 18, 2020

@atollk I would not call the use of setup.py to be the "canonical way" of managing python packages. It has most certainly been the defacto option for a long while. However with the introduction of PEP 517 and PEP 518, the python packaging ecosystem is building towards a "standards" based approach that detaches the tool specifics from the package build-system configuration. The use of setup.py simply indicates the use of setuptools as the PEP 517 build backend. For poetry managed packages we expect poetry to be used as the build backend. For context, pip supports this from version 19.1. TLDR; python package managers should be able to build packages defining build systems via pyproject.toml.

As for PEP 561, by the time the setup file generation is removed, we anticipate the required features to be supported. For example see #2000. This needs to be ported to core, but I do expect to see the py.typed usecase supported soon. Hope that helps.

@bdoms
Copy link
Author

bdoms commented Jul 22, 2020

@abn just wanted say a big THANK YOU! I really appreciate the well thought out responses. Poetry is in good hands.

br3ndonland added a commit to br3ndonland/inboard that referenced this issue Sep 12, 2020
https://www.python.org/dev/peps/pep-0561/
https://mypy.readthedocs.io/en/latest/installed_packages.html
python-poetry/poetry#1338

The py.typed file indicates that this package supplies type information.
py.typed is not yet supported by Poetry (python-poetry/poetry#1338), so
it may not show up in the built package yet.
br3ndonland added a commit to br3ndonland/inboard that referenced this issue Sep 12, 2020
https://www.python.org/dev/peps/pep-0561/
https://mypy.readthedocs.io/en/latest/installed_packages.html
python-poetry/poetry#1338

The py.typed file indicates that this package supplies type information.
py.typed is not yet supported by Poetry (python-poetry/poetry#1338), so
it may not show up in the built package yet.
br3ndonland added a commit to br3ndonland/inboard that referenced this issue Sep 12, 2020
https://www.python.org/dev/peps/pep-0561/
https://mypy.readthedocs.io/en/latest/installed_packages.html
python-poetry/poetry#1338

The py.typed file indicates that this package supplies type information.
py.typed is not yet supported by Poetry (python-poetry/poetry#1338), so
it may not show up in the built package yet.
@br3ndonland
Copy link

py.typed now appears to be properly included after packaging my project with Poetry 1.1.0 and poetry-core 1.0.0.

# pyproject.toml
[tool.poetry]
include = ["mypackage/py.typed"]

[build-system]
requires = ["poetry-core>=1.0.0"]
build-backend = "poetry.core.masonry.api"

To verify without affecting your PyPI package, you can publish to Test PyPI as described in the PyPA tutorial.

Thank you to @abn and the other maintainers and contributors 🎉 👏

@abn
Copy link
Member

abn commented Oct 5, 2020

@bdoms safe to say original issue is resolved?

TeoZosa added a commit to TeoZosa/cookiecutter-cruft-poetry-tox-pre-commit-ci-cd that referenced this issue Jan 17, 2021
From [Hypermodern Python Cookiecutter](https://cookiecutter-hypermodern-python.readthedocs.io/en/2020.6.15/guide.html?highlight=py.typed#the-initial-package):
"`py.typed` is an empty marker file, which declares that your package
supports typing and is distributed with its own type information (PEP
561). This allows people using your package to type-check their Python
code against it."

See:
- [PEP-561](https://www.python.org/dev/peps/pep-0561/)
- [Reference implementation: `aws-lambda-powertools-python`](aws-powertools/powertools-lambda-python#237)
- [Implementation Explanation](python-poetry/poetry#1338 (comment))
@bdoms
Copy link
Author

bdoms commented Jun 24, 2021

Apologies for not responding earlier - I had checked for this bug last October after that release, and found that it was still happening.

However, I did just check again after an update and can confirm that at least as of Poetry version 1.1.6 this is finally fixed!

THANK YOU!!!

Copy link

github-actions bot commented Mar 2, 2024

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind/bug Something isn't working as expected
Projects
None yet
Development

Successfully merging a pull request may close this issue.