Skip to content

Correct types for setuptools.setup #12791

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

Merged
merged 7 commits into from
Oct 13, 2024

Conversation

max-muoto
Copy link
Contributor

Fixes #12752

@max-muoto max-muoto marked this pull request as ready for review October 12, 2024 18:42

This comment has been minimized.

@@ -45,6 +45,13 @@ __all__ = [

__version__: str

class _Library(TypedDict):
Copy link
Member

Choose a reason for hiding this comment

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

Should this be marked @type_check_only? There seems to be some precedence for using it on some similar typeddicts, e.g.

@type_check_only
class _LocaleConv(TypedDict):

Also, to bikeshed the name, I see setuptools internally calls this structure a build_info in a few places, so maybe _BuildInfo would be a good fit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting, I wasn't aware of it, but likely what we want here. Thanks for the callout. Agree that's a better name here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This comment has been minimized.

Comment on lines 49 to 55
@type_check_only
class _BuildInfo(TypedDict):
sources: list[str] | tuple[str]
obj_deps: NotRequired[dict[str, list[str] | tuple[str]]]
macros: NotRequired[list[tuple[str, str] | tuple[str]]]
include_dirs: NotRequired[list[str]]
cflags: NotRequired[list[str]]
Copy link
Collaborator

@Avasam Avasam Oct 12, 2024

Choose a reason for hiding this comment

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

Normally I'd prefer defining such types close to where their implementation stands (so with the appropriate Command or Distribution) but in this case it'd end up in distutils and/or setuptools/_distutils so I guess in setuptools.__init__.py is fine.

I do have a concern using a type-only TypedDict with required members as a parameter: It makes it annoying to use a non-literal dict. Maybe that's ok in setuptools due to the nature of it looking like a config file, it shouldn't be much more complex than my example below?

It's possible, but requires a private type-only import, which isn't great:

from typing import cast, TYPE_CHECKING
if TYPE_CHECKING:
    from setuptools import _BuildInfo

libs = [
    ("lib1", {"sources": []}),
    ("lib2", {"sources": []}),
]
setup(libraries=libs)  # Type error in both mypy and pyright due to missing "sources"
# Ok, but must quote type to avoid runtime error
setup(libraries=cast(list[tuple[str, "_BuildInfo"]], libs))
# Ok
setup(libraries=[("literal", {"sources": []})])
# Ok
libs_annotated: list[tuple[str, "_BuildInfo"]] = [ # Must quote type or use from __future__ import annotations
    ("lib1", {"sources": []}),
    ("lib2", {"sources": []}),
]
setup(libraries=libs_annotated)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, perhaps it's not ideal but I'm not seeing a great alternative, obviously passing things in-line solves this problem, or just:

libs: "list[tuple[str, _BuildInfo]]" = [
    ("lib1", {"sources": []}),
    ("lib2", {"sources": []}),
]
setup(libraries=libs)

Which would work without quotes given deferred annotations.

Copy link
Collaborator

Choose a reason for hiding this comment

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

At the same time, the kind of people to type-check their setup.py are probably the same going all out on typing (looking at myself here ^^)

Co-authored-by: Avasam <samuel.06@hotmail.com>

This comment has been minimized.

@@ -45,6 +46,14 @@ __all__ = [

__version__: str

@type_check_only
class _BuildInfo(TypedDict):
sources: list[str] | tuple[str]
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure this isn't tuple[str, ...]? In other words, is this really always a one-tuple? (A few more times below.)

Copy link
Collaborator

@Avasam Avasam Oct 13, 2024

Choose a reason for hiding this comment

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

You're right, I also glanced over that...
(same for all definitions below)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The one tuple for macros is correct, but the others were a mistake: 2bb4ed4

Copy link
Collaborator

@Avasam Avasam left a comment

Choose a reason for hiding this comment

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

Oops, I also glanced over this: tuple[str] means a single-item tuple, we want tuple[str, ...]
https://github.com/python/typeshed/pull/12791/files#r1797961597

@max-muoto
Copy link
Contributor Author

Oops, I also glanced over this: tuple[str] means a single-item tuple, we want tuple[str, ...] https://github.com/python/typeshed/pull/12791/files#r1797961597

Yep, I assumed they were actually one-tuples from the original issue but it looks like macros is the only one that should be: https://github.com/python/typeshed/pull/12791/files#r1798508136

@max-muoto
Copy link
Contributor Author

Oops, I also glanced over this: tuple[str] means a single-item tuple, we want tuple[str, ...] https://github.com/python/typeshed/pull/12791/files#r1797961597

Additionally the second value in the tuple can be None: 82ad6cb

Here's a good example of what the types are allowed to be at runtime.

Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@Avasam Avasam requested a review from JelleZijlstra October 13, 2024 19:26
@JelleZijlstra JelleZijlstra merged commit 407eeaa into python:main Oct 13, 2024
48 checks passed
Yay295 added a commit to Yay295/Pillow that referenced this pull request Oct 28, 2024
Yay295 added a commit to Yay295/Pillow that referenced this pull request Oct 28, 2024
eyedav added a commit to eyedav/Pillow that referenced this pull request Jul 19, 2025
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.

setuptools.setup() libraries parameter is wrong
4 participants