Skip to content

Mimalloc header is not installed #116984

Closed
Closed
Mimalloc header is not installed#116984
Labels
type-bugAn unexpected behavior, bug, or error

Description

@oraluben
oraluben
opened on Mar 19, 2024

Bug report

Bug description:

Mimalloc is introduced in #109914.
This will cause a header not found when building any extension that includes a pycore_*.h header as:

[build] In file included from /Users/yyc/repo/py/install/include/python3.13d/internal/pycore_long.h:13:
[build] In file included from /Users/yyc/repo/py/install/include/python3.13d/internal/pycore_runtime.h:17:
[build] In file included from /Users/yyc/repo/py/install/include/python3.13d/internal/pycore_interp.h:30:
[build] /Users/yyc/repo/py/install/include/python3.13d/internal/pycore_mimalloc.h:39:10: fatal error: 'mimalloc.h' file not found
[build] #include "mimalloc.h"
[build]          ^~~~~~~~~~~~
[build] 1 error generated.

See following PR for detail.

CPython versions tested on:

3.13, CPython main branch

Operating systems tested on:

Linux, macOS

Linked PRs

Activity

  1. oraluben
    added
    type-bugAn unexpected behavior, bug, or error
    on Mar 19, 2024

  2. pitrou commented on Mar 19, 2024

    @pitrou
    pitrou
    on Mar 19, 2024
    Member

    Slightly unrelated, but I see that mimalloc types are embedded directly in structures such as PyInterpreterState or PyThreadState.

    Unless mimalloc has a stable ABI, does it risk breaking if an extension module or an application embedding Python uses its own different version of mimalloc?

    It's also weird that a third-party library is included from CPython headers. Perhaps it would be better to hide this behind opaque pointers.

    @encukou @colesbury

  3. oraluben commented on Mar 19, 2024

    @oraluben
    oraluben
    on Mar 19, 2024
    ContributorAuthor

    I walked through the discussions in #109914 and seems this issue has been already discussed (at least mentioned, but forgotten later? Or not correctly installed after moved to Internal/? #109914 (review), #109914 (comment), #109914 (comment)). It's somehow intended that this header is not installed, but since cpython decides to install pycore_*.h headers, the mimalloc headers need to be installed as well (otherwise the core headers are all broken).

    I also see some related about this:

    Unless mimalloc has a stable ABI, does it risk breaking if an extension module or an application embedding Python uses its own different version of mimalloc?

    If I'm not mistaken, no symbol is exported (#109914 (comment)), and the reason mimalloc headers are included is that there's extra patches on top of upstream. But the longterm plan is to upstream those (I cannot find the link to this but I believe saw Sam mentioned it somewhere).

    Hope it helped @pitrou

  4. pitrou commented on Mar 19, 2024

    @pitrou
    pitrou
    on Mar 19, 2024
    Member

    If I'm not mistaken, no symbol is exported

    Sure, but:

    1. mimalloc.h is included, and it could come from whatever install of mimalloc is used by third-party extension code, not necessarily the mimalloc used for building CPython
    2. the binary layout of PyInterpreterState and PyThreadState depends on the size of mimalloc types, which may vary from one mimalloc version to another.

  5. colesbury commented on Mar 19, 2024

    @colesbury
    colesbury
    on Mar 19, 2024
    Contributor

    @pitrou:

    1. mimalloc.h is only included in internal headers (pycore_xxx.h). Not in any of the public APIs (Include or Include/cpython).
    2. PyInterpreterState is opaque. The mimalloc structs are included in _PyThreadStateImpl, not PyThreadState. _PyThreadStateImpl is not in any of the public headers.

  6. pitrou commented on Mar 19, 2024

    @pitrou
    pitrou
    on Mar 19, 2024
    Member

    @colesbury This issue shows that "internal" headers are included by third-party code. Presumably, PyInterpreterState and PyThreadState are also consumed by third-party code.

  7. colesbury commented on Mar 19, 2024

    @colesbury
    colesbury
    on Mar 19, 2024
    Contributor

    Yes, people sometimes include our internal headers. People also sometimes copy-paste our internal headers. We don't make attempts to avoid language level name conflicts in these cases.

  8. pitrou commented on Mar 19, 2024

    @pitrou
    pitrou
    on Mar 19, 2024
    Member

    I certainly don't have a horse in this race, but I find it weird to on the one hand fix this issue by installing the mimalloc headers, and on the other hand to not care about the potential consequences of including said headers in third-party code.

  9. oraluben commented on Mar 20, 2024

    @oraluben
    oraluben
    on Mar 20, 2024
    ContributorAuthor

    I'd also prefer to not need to add Internal/mimalloc as extra include path, even after the headers are installed, if that's possible.

  10. erlend-aasland
    added a commit that references this issue on Apr 23, 2024

    gh-116984: Install mimalloc headers (#116985)

    e17cd1f
  11. erlend-aasland
    closed this as completedon Apr 23, 2024

  12. colesbury commented on May 8, 2024

    @colesbury
    colesbury
    on May 8, 2024
    Contributor

    Reopening this for the Internal/mimalloc include path issue.

  13. colesbury
    reopened this on May 8, 2024
  14. colesbury
    added 3 commits that reference this issue on May 8, 2024

    pythongh-116984: Make mimalloc header includes relative to the curren…

    503bda7

    pythongh-116984: Make mimalloc header includes relative to the curren…

    74ebbee

    gh-116984: Make mimalloc header includes relative to the current file (…

    71cc065
  15. miss-islington
    added a commit that references this issue on May 9, 2024

    pythongh-116984: Make mimalloc header includes relative to the curren…

    41973dc
  16. colesbury
    added a commit that references this issue on May 9, 2024

    [3.13] gh-116984: Make mimalloc header includes relative to the curre…

    0a23970
  17. SonicField
    added a commit that references this issue on May 10, 2024

    pythongh-116984: Make mimalloc header includes relative to the curren…

    e0bb384

  18. oraluben commented on May 11, 2024

    @oraluben
    oraluben
    on May 11, 2024
    ContributorAuthor

    Closing for the PRs from @colesbury have been landed, thanks for make this more friendly!

  19. oraluben
    closed this as completedon May 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    type-bugAn unexpected behavior, bug, or error

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions