python-poetry / poetry

Python packaging and dependency management made easy
https://python-poetry.org
MIT License
31.1k stars 2.25k forks source link

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

Closed bdoms closed 3 years ago

bdoms commented 5 years ago

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 commented 5 years ago

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()
        )
con-f-use commented 5 years ago

see: https://github.com/pypa/setuptools/pull/1650

funkyfuture commented 5 years ago

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[bot] commented 4 years ago

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.

bdoms commented 4 years ago

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.

sdispater commented 4 years ago

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

gijzelaerr commented 4 years ago

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

bdoms commented 4 years ago

@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 commented 4 years ago

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 https://github.com/ActivityWatch/aw-qt/pull/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 commented 4 years ago

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 commented 4 years ago

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

KaiserKarel commented 4 years ago

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 commented 4 years ago

same here with version 1.0.9

abn commented 4 years ago

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/core#26. However, note that there is a bug in the implementation that will be resolved once python-poetry/core#43 is merged. In 1.1 this will be opt-in, and in 1.2 this will become default behaviour.

sebix commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@bdoms We already know it's in 1.0.9 (https://github.com/python-poetry/poetry/issues/1338#issuecomment-647248289).

FWIW, my particular variant of the issue (as described in https://github.com/python-poetry/poetry/issues/1338#issuecomment-571618450) appears to have been resolved in 1.1.0a3.

atollk commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@abn I see. I added a PR which should fix the issue. The code in question is what was described above: https://github.com/python-poetry/poetry/issues/1338#issuecomment-554450715 .

atollk commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

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

br3ndonland commented 3 years ago

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 :tada: :clap:

abn commented 3 years ago

@bdoms safe to say original issue is resolved?

bdoms commented 3 years ago

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!!!

github-actions[bot] commented 6 months ago

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.