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

Inconsistent default behavior of include #9691

Closed
radoering opened this issue Sep 16, 2024 · 3 comments · Fixed by #9734
Closed

Inconsistent default behavior of include #9691

radoering opened this issue Sep 16, 2024 · 3 comments · Fixed by #9734
Labels
area/build-system Related to PEP 517 packaging (see poetry-core) area/core Related to the poetry-core library kind/bug Something isn't working as expected

Comments

@radoering
Copy link
Member

Before diving into the issue, a short clarification:

  1. There are packages includes, which default to sdist and wheel.
  2. There are includes, which (according to the docs) default to only sdist.

These different defaults can be confusing, but might make sense because you normally want to include packages in sdists and wheels but additional data like tests and a changelog only in the sdist.

Issue

The behavior for includes (the second one) is different for files and directories:

  • files are included in both sdist and wheel
  • directories are only included in sdists

Considering that the docs were "fixed" recently in #8852 to describe the behavior for directories - previously, it described the behavior for files - I wondered which behavior is correct and which is the bug?

Events of confusion in chronologically order:

We can be sure that the initial intention was to default to "only sdist". However, I am not sure if the behavior should be changed to "sdist and wheel" later or if it was just a confusion between the two types of includes.

Thus, the big question: What should the default be?

  • sdist and wheel for package includes (first type) and sdist only for includes (second type)
    • pro: You normally want to include packages in sdists and wheels but additional data like tests and a changelog only in the sdist
    • contra: different defaults
  • sdist and wheel for both types of includes
    • pro: same defaults
    • contra: maybe improper default for one type
@radoering radoering added kind/bug Something isn't working as expected area/build-system Related to PEP 517 packaging (see poetry-core) area/core Related to the poetry-core library labels Sep 16, 2024
@radoering
Copy link
Member Author

After taking a closer look at it, here are a few more arguments:

default to "only sdist"

  • When installing a wheel with includes, the includes are unpacked straight into site-packages, which does not makes sense for a top level changelog or tests. (thanks @Secrus)

default to "sdist and wheel"

So it looks like there are good reasons for either direction...

In general, I think it is easier to understand that all sorts of include (and exlcude) default to all formats (i.e. sdist and wheel). However, I also see the danger that this results in changelogs and tests being unintentionally included in wheels.

@radoering
Copy link
Member Author

@johnthagen Since you often give good advice on usability and this is a difficult choice, I'd appreciate your opinion. (In case, you have never used this feature and have no opinion that is also fine, of course.)

@johnthagen
Copy link
Contributor

johnthagen commented Oct 4, 2024

@radoering Thanks for the ping. I primarily use Poetry for applications rather than for packaging sdists/wheels for libraries. Because of this, I have not used these features of Poetry before so my opinion is not very relevant.

For example, I was a big fan of

Because this is actually how I use Poetry these days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/build-system Related to PEP 517 packaging (see poetry-core) area/core Related to the poetry-core library kind/bug Something isn't working as expected
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants