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

Update Modules/Setup #89711

Closed
brettcannon opened this issue Oct 20, 2021 · 31 comments
Closed

Update Modules/Setup #89711

brettcannon opened this issue Oct 20, 2021 · 31 comments
Assignees
Labels
3.11 only security fixes build The build process and cross-build

Comments

@brettcannon
Copy link
Member

BPO 45548
Nosy @malemburg, @brettcannon, @tiran, @ned-deily, @zware, @asottile, @miss-islington
PRs
  • bpo-45548: add some missing entries to Modules/Setup #29115
  • bpo-45548: Make Modules/Setup easier to read #29143
  • bpo-45548: Have Modules/Setup only try to compile _math once #29177
  • bpo-45548: Remove _math.c workarounds for pre-C99 libm #29179
  • bpo-45548: Inline _math.c into _math.h #29181
  • bpo-45548: Put Modules/Setup extensions into builddir #29188
  • bpo-45548: Add missing extensions to Modules/Setup #29199
  • bpo-45548: Remove checks for finite and gamma (GH-29206) #29206
  • bpo-45548: FreeBSD doesn't like auto vars in makesetup (GH-29216) #29216
  • bpo-45548: makesetup improvements (GH-29225) #29225
  • bpo-45548: Fix out-of-tree and Debian builds (GH-29263) #29263
  • bpo-45548: Some test modules must be built as shared libs (GH-29268) #29268
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/brettcannon'
    closed_at = <Date 2021-11-23.10:58:45.780>
    created_at = <Date 2021-10-20.23:58:48.199>
    labels = ['build', '3.11']
    title = 'Update Modules/Setup'
    updated_at = <Date 2021-11-23.10:58:45.779>
    user = 'https://github.com/brettcannon'

    bugs.python.org fields:

    activity = <Date 2021-11-23.10:58:45.779>
    actor = 'christian.heimes'
    assignee = 'brett.cannon'
    closed = True
    closed_date = <Date 2021-11-23.10:58:45.780>
    closer = 'christian.heimes'
    components = ['Build']
    creation = <Date 2021-10-20.23:58:48.199>
    creator = 'brett.cannon'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 45548
    keywords = ['patch']
    message_count = 31.0
    messages = ['404548', '404559', '404560', '404627', '404710', '404790', '404864', '404897', '404898', '404899', '404900', '404906', '404924', '404953', '404980', '404987', '404992', '405032', '405045', '405046', '405047', '405053', '405055', '405056', '405092', '405123', '405172', '405227', '405314', '406812', '406834']
    nosy_count = 7.0
    nosy_names = ['lemburg', 'brett.cannon', 'christian.heimes', 'ned.deily', 'zach.ware', 'Anthony Sottile', 'miss-islington']
    pr_nums = ['29115', '29143', '29177', '29179', '29181', '29188', '29199', '29206', '29216', '29225', '29263', '29268']
    priority = 'low'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue45548'
    versions = ['Python 3.11']

    @brettcannon
    Copy link
    Member Author

    [Modules/Setup](https://github.com/python/cpython/blob/main/Modules/Setup) is currently incomplete/broken. Some things are completely missing from it, while others won't work if you uncomment them in the file.

    When trying to compile a completely static CPython interpreter I ran into the following issues:

    • _weakref is listed twice
    • _testcapi can't be statically compiled
    • math/cmath conflict with a build rule in Makefile.pre.in
    • _dbm needs more logic/comments (at least on macOS)
    • nis needs more logic/comments (at least on macOS)

    And the following modules are completely missing from the file:

    • _ctypes
    • _decimal
    • _lsprof
    • _lzma
    • _multiprocessing
    • _opcode
    • _posixshmem
    • _queue
    • _scproxy
    • _sqlite3
    • _testbuffer
    • _testipmortmultiple
    • _testmultiphase
    • _uuid
    • xxsubinterpreters
    • xxtestfuzz
    • ossaudiodev
    • xxlimited
    • xxlimited_35

    @brettcannon brettcannon added the 3.11 only security fixes label Oct 20, 2021
    @brettcannon brettcannon self-assigned this Oct 20, 2021
    @brettcannon brettcannon added build The build process and cross-build 3.11 only security fixes labels Oct 20, 2021
    @brettcannon brettcannon self-assigned this Oct 20, 2021
    @brettcannon brettcannon added the build The build process and cross-build label Oct 20, 2021
    @miss-islington
    Copy link
    Contributor

    New changeset dd86f63 by Brett Cannon in branch 'main':
    bpo-45548: add some missing entries to [Modules/Setup](https://github.com/python/cpython/blob/main/Modules/Setup) (GH-29115)
    dd86f63

    @brettcannon
    Copy link
    Member Author

    Status is now:

    • math/cmath conflict with a build rule in Makefile.pre.in
    • _dbm needs more logic/comments (at least on macOS)
    • nis needs more logic/comments (at least on macOS)

    And the following modules are completely missing from the file:

    • _bz2
    • _ctypes
    • _decimal
    • _multiprocessing
    • _posixshmem
    • _scproxy
    • _sqlite3
    • _uuid

    @tiran
    Copy link
    Member

    tiran commented Oct 21, 2021

    I started https://github.com/python/cpython/compare/main...tiran:configure_pkgconfig?expand=1 to integrate pkg-config with configure. The patchset also contains examples how to pass flags from configure into Makefile and Modules/Setup.

    @brettcannon
    Copy link
    Member Author

    New changeset 01cf4fb by Brett Cannon in branch 'main':
    bpo-45548: Make [Modules/Setup](https://github.com/python/cpython/blob/main/Modules/Setup) easier to read (GH-29143)
    01cf4fb

    @tiran
    Copy link
    Member

    tiran commented Oct 22, 2021

    I added several missing module definitions in #29164

    math/cmath conflict when _math.c is included twice.

    @tiran
    Copy link
    Member

    tiran commented Oct 23, 2021

    I ran into another problem. In shared mode, Modules/Setup places the shared modules in ./Modules/ instead of the build directory ./build/lib.something/. The ./Modules/ directory is not on sys.path. getpath.c only adds the build dir to sys.path. It takes the value from ./pybuilddir.txt. The file is generated by sysconfig.py.

    $ make
    ...
    *** WARNING: renaming "_asyncio" since importing it failed: No module named 'math'

    The following modules found by detect_modules() in setup.py, have been
    built by the Makefile instead, as configured by the Setup files:
    _abc cmath math
    pwd time

    $ find -name 'math*.so'
    ./Modules/math.cpython-311-x86_64-linux-gnu.so

    @malemburg
    Copy link
    Member

    FYI: I've been working with a fixed Setup file in PyRun for a long while. There are indeed a number of modules missing from Setup, since the whole logic was left behind a bit after things moved to setup.py.

    The issue with _math.o is actually in the main Makefile.pre.in. The version listed there does not match the Makefile lines added via Setup. For PyRun, I had to comment out the one in Makefile.pre.in and then only add one instance to the math module and not the cmath one. This avoids a (harmless) warning during the build.

    I'm not sure what the _math.o entry exists in Makefile.pre.in. It's only needed by those two modules, AFAIK.

    Here's the list of modules I had to add in the past (taken from the 3.10 port):

    """
    ### Built-in extensions for which there are no entries in Setup.dist/Setup:

    # _decimal needs more complex setup, punting on this for now
    #DECIMAL_DEFS=-DCONFIG_64=1 -DASM=1
    #_decimal \
    # _decimal/_decimal.c \
    # _decimal/libmpdec/basearith.c \
    # _decimal/libmpdec/constants.c \
    # _decimal/libmpdec/context.c \
    # _decimal/libmpdec/convolute.c \
    # _decimal/libmpdec/crt.c \
    # _decimal/libmpdec/difradix2.c \
    # _decimal/libmpdec/fnt.c \
    # _decimal/libmpdec/fourstep.c \
    # _decimal/libmpdec/io.c \
    # _decimal/libmpdec/memory.c \
    # _decimal/libmpdec/mpdecimal.c \
    # _decimal/libmpdec/numbertheory.c \
    # _decimal/libmpdec/sixstep.c \
    # _decimal/libmpdec/transpose.c \
    # $(DECIMAL_DEFS) \
    # -I$(srcdir)/Modules/_decimal \
    # -I$(srcdir)/Modules/_decimal/libmpdec \
    # -I$(prefix)/include -L$(exec_prefix)/lib

    # _opcode
    _opcode _opcode.c

    # _ctypes needs to build libffi first - punting on this

    # _lsprof
    _lsprof _lsprof.c rotatingtree.c

    # _sqlite3
    SQLITE_DEFS=-DMODULE_NAME='"sqlite3"' -DSQLITE_OMIT_LOAD_EXTENSION
    # @if freebsd: SQLITE_LIBS=-I/usr/local/include -L/usr/local/lib
    # @if not freebsd: SQLITE_LIBS=
    _sqlite3 \
    _sqlite/module.c \
    _sqlite/cache.c \
    _sqlite/connection.c \
    _sqlite/cursor.c \
    _sqlite/microprotocols.c \
    _sqlite/prepare_protocol.c \
    _sqlite/row.c \
    _sqlite/statement.c \
    _sqlite/util.c \
    $(SQLITE_DEFS) -I$(srcdir)/Modules/_sqlite \
    $(SQLITE_LIBS) \
    -I$(prefix)/include -L$(exec_prefix)/lib \
    -lsqlite3

    # bz2
    _bz2 _bz2module.c -lbz2

    # lzma

    # Note: Adding this can cause serious issues, since the needed lib isn't
    # universally installed everywhere. See bpo-1793 and bpo-1794.

    #_lzma _lzmamodule.c -llzma

    # multiprocessing
    _multiprocessing \
    _multiprocessing/semaphore.c \
    _multiprocessing/multiprocessing.c \
    -I$(srcdir)/Modules/_multiprocessing

    # Optional add-on for multiprocessing to use shared memory
    #POSIXSHMEM_LIBS=rt
    POSIXSHMEM_LIBS=
    _posixshmem \
            _multiprocessing/posixshmem.c \
            -I$(srcdir)/Modules/_multiprocessing \
            $(POSIXSHMEM_LIBS)

    # queue
    _queue _queuemodule.c
    """

    Not all modules are included, since I did not need all missing ones.

    @tiran
    Copy link
    Member

    tiran commented Oct 23, 2021

    PR #73365 or #73367 address the issue with _math.o

    PR #73350 adds the missing modules and also introduces pkg-config lookups for dependencies.

    Brett and I discussed that we have to introduce conditionals to Modules/Setup to make use of the AM_CONDITIONALs from PR #73350. We also have to figure out how to deal with system-libmpdec and system-cffi.

    @malemburg
    Copy link
    Member

    I'm using a very simple conditional logic in Setup, which is based
    on sed, mostly to add platform specific variables:

    In Setup:

    # @if macosx: TIME_DEFS=
    # @if not macosx: TIME_DEFS=-lrt
    time -DPy_BUILD_CORE_BUILTIN -I$(srcdir)/Include/internal timemodule.c
    $(TIME_DEFS) # -lm # time operations and variables

    In Makefile:

            # Install the custom Modules/Setup file
            if test "$(MACOSX_PLATFORM)"; then \
                    sed     -e 's/# @if macosx: *//' \
                            $(PYRUNDIR)/$(MODULESSETUP) > $(PYTHONDIR)/Modules/Setup; \
            elif test "$(FREEBSD_PLATFORM)"; then \
                    sed     -e 's/# @if freebsd: *//' \
                            $(PYRUNDIR)/$(MODULESSETUP) > $(PYTHONDIR)/Modules/Setup; \
            else \
                    sed     -e 's/# @if not macosx: *//' \
                            -e 's/# @if not freebsd: *//' \
                            $(PYRUNDIR)/$(MODULESSETUP) > $(PYTHONDIR)/Modules/Setup; \
            fi;

    Setup used to be templated as well in the past, but that logic was removed
    at some point. Perhaps it's time to reintroduce it.

    @malemburg
    Copy link
    Member

    On 23.10.2021 20:04, Christian Heimes wrote:

    PR #73365 or #73367 address the issue with _math.o

    I think those patches are both taking things a bit too far.

    This is a build problem, not a code problem. It's perfectly
    good style to link a single file to multiple other object files
    instead of copying the code into those object files.

    The catch is that the makesetup logic is not smart enough to only
    include the necessary Makefile line once.

    The entry in Makefile.pre.in should not be needed, since the logic
    for building math and cmath modules already exists in setup.py.

    BTW: There's a simple trick to avoid the makesetup issue: simply
    add the _math.c entry to some other module which is always linked
    statically, e.g. _stat, and remove it from both math and cmath
    entries in Setup, as well as the Makefile.pre.in.

    @tiran
    Copy link
    Member

    tiran commented Oct 23, 2021

    The trick would move the math function back into the core. Mark moved the math functions out of the core on purpose, see bpo-7518.

    @malemburg
    Copy link
    Member

    On 23.10.2021 21:30, Christian Heimes wrote:

    The trick would move the math function back into the core. Mark moved the math functions out of the core on purpose, see bpo-7518.

    I don't follow you. With the _math.o target in Makefile.pre.in,
    _math.c was always compiled into the main Python interpreter,
    even with math and cmath built as shared libs.

    And yes, it does export a symbol, but _Py_log1p is not going to
    conflict with anything else out there :-)

    The trick is essentially not changing the 3.10 status quo. It
    only makes sure that _math.o is compiled in the same way as all
    other Setup managed modules and moves the target from Makefile.pre.in
    to the makesetup section of the final Makefile.

    And it removes the warning of having multiple _math.o targets
    ending up in the Makefile, which isn't problematic, since make
    will always use the last definition (from the makesetup section),
    but doesn't look nice either.

    OTOH, _math.h and .c are really small, so perhaps it's better
    to merge both into a single _math.h file and include that directly
    into the modules. I believe that's what Brett's patch does, right ?

    Today, only the _Py_log1p() code is actually used and then only
    to address a very special case in a platform independent way
    (log1p(-0.0) == -0.0), so the whole code boils down to some 10
    lines of C being incorporated.

    @miss-islington
    Copy link
    Contributor

    New changeset fa26245 by Christian Heimes in branch 'main':
    bpo-45548: Remove _math.c workarounds for pre-C99 libm (GH-29179)
    fa26245

    @tiran
    Copy link
    Member

    tiran commented Oct 25, 2021

    New changeset 77e3f22 by Christian Heimes in branch 'main':
    bpo-45548: Remove checks for finite and gamma (GH-29206)
    77e3f22

    @tiran
    Copy link
    Member

    tiran commented Oct 25, 2021

    New changeset ece916e by Christian Heimes in branch 'main':
    bpo-45548: Add missing extensions to Modules/Setup (GH-29199)
    ece916e

    @tiran
    Copy link
    Member

    tiran commented Oct 25, 2021

    New changeset 2b8677a by Christian Heimes in branch 'main':
    bpo-45548: FreeBSD doesn't like auto vars in makesetup (GH-29216)
    2b8677a

    @tiran
    Copy link
    Member

    tiran commented Oct 26, 2021

    New changeset b5ee794 by Christian Heimes in branch 'main':
    bpo-45548: makesetup improvements (GH-29225)
    b5ee794

    @tiran
    Copy link
    Member

    tiran commented Oct 26, 2021

    Brett, we can use AM_CONDITIONAL() to conditionally enable/disable a feature and AC_CONFIG_FILES() to create a Modules/Setup from a template:

    Example:

    The conditional

        AM_CONDITIONAL([HAVE_SCPROXY], [test "$ac_sys_system" = "Darwin"])

    sets HAVE_SCPROXY_FALSE and HAVE_NIS_SCPROXY based on the check.

    On macOS:

    HAVE_SCPROXY_FALSE='#'
    HAVE_SCPROXY_TRUE=''
    

    On Linux:

    HAVE_SCPROXY_FALSE=''
    HAVE_SCPROXY_TRUE='#'
    

    We can either do something like:

    *shared*
    @HAVE_SCPROXY_TRUE@_scproxy _scproxy.c -framework SystemConfiguration -framework CoreFoundation
    

    or:

    *disabled*
    @HAVE_SCPROXY_FALSE*_scproxy
    

    @tiran
    Copy link
    Member

    tiran commented Oct 26, 2021

    Typo, the last line should read "@HAVE_SCPROXY_FALSE@_scproxy"

    @malemburg
    Copy link
    Member

    On 26.10.2021 16:17, Christian Heimes wrote:

    Christian Heimes <lists@cheimes.de> added the comment:

    Brett, we can use AM_CONDITIONAL() to conditionally enable/disable a feature and AC_CONFIG_FILES() to create a Modules/Setup from a template:

    Example:

    The conditional

    AM_CONDITIONAL([HAVE_SCPROXY], [test "$ac_sys_system" = "Darwin"])
    

    sets HAVE_SCPROXY_FALSE and HAVE_NIS_SCPROXY based on the check.

    I think it would be more helpful (and generate a lot fewer such
    macros), if we'd just test for platforms and then use those in the
    Setup.in template.

    Some other things:

    I saw that you stripped off lots of -I and -L options from the Setup
    lines. Are those now unconditionally taken from somewhere else,
    without possibility to override them ?

    You also removed the structure of the listings, which makes things
    like dependencies between e.g. _multibytecodec and the CJK codecs
    unclear, the notes about audio, the comments on building the _md5
    and sha* modules, etc.

    Since Setup is a (potentially) hand edited file, it's good to leave
    as much useful information in that file as possible, to not
    accidentally create setups which don't work and to have references
    which may help in finding the right compile options.

    @tiran
    Copy link
    Member

    tiran commented Oct 26, 2021

    Brett removed a lot of stuff in 01cf4fb to make the file more readable. I removed unnecessary -D, -I, and -L to make the file even more readable. You can pass custom flags to ./configure.

    Setup should not be edited by hand. Customizations go to Setup.local.

    @malemburg
    Copy link
    Member

    On 26.10.2021 19:42, Christian Heimes wrote:

    Brett removed a lot of stuff in 01cf4fb to make the file more readable. I removed unnecessary -D, -I, and -L to make the file even more readable. You can pass custom flags to ./configure.

    Could Brett or you please add those notes back ? There's no other place
    where such details are documented. We've lost important information and
    I would like to get this back into Setup and ideally add more
    information to make it easier for users or admins to customize
    their build.

    Regarding the -I and -L flags, my question was whether these are
    now added elsewhere, since the code would not compile without
    e.g. -I$(srcdir)/Include/internal.

    I've gone through the Makefile and found that these are already
    added via PY_CFLAGS_NODIST, so they are indeed not needed in Setup.
    That's great :-)

    Please be careful when moving e.g. -I or -L options which point to
    non-Python directories. If such options get moved into e.g.
    PY_CFLAGS_NODIST, there's no way to override them via options
    given in Setup.

    Having the -D defined in the relevant code is a lot better. Thanks
    for that :-)

    Setup should not be edited by hand. Customizations go to Setup.local.

    Editing Setup.local instead of Setup is not documented anywhere ?!
    Both files are read, so I guess it's not all that relevant which
    of the two are edited.

    @malemburg
    Copy link
    Member

    On 26.10.2021 20:47, Marc-Andre Lemburg wrote:

    > Brett removed a lot of stuff in 01cf4fb to make the file more readable. I removed unnecessary -D, -I, and -L to make the file even more readable. You can pass custom flags to ./configure.

    Could Brett or you please add those notes back ? There's no other place
    where such details are documented. We've lost important information and
    I would like to get this back into Setup and ideally add more
    information to make it easier for users or admins to customize
    their build.

    I could also edit the file and add those back, after you're done
    with the refactoring.

    @asottile
    Copy link
    Mannequin

    asottile mannequin commented Oct 27, 2021

    I'm seeing some weird breakage in the deadsnakes builds, presumably due to this change:

    ...
    2021-10-27T08:55:21.9485959Z x86_64-linux-gnu-gcc -pthread -Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O3 -Wall -g -fdebug-prefix-map=/tmp/code=. -specs=/usr/share/dpkg/no-pie-compile.specs -fstack-protector -Wformat -Werror=format-security    -std=c99 -Wextra -Wno-unused-result -Wno-unused-parameter -Wno-missing-field-initializers -Werror=implicit-function-declaration -fvisibility=hidden  -I../Include/internal -IObjects -IInclude -IPython -I. -I../Include -Wdate-time -D_FORTIFY_SOURCE=2   -DPy_BUILD_CORE_BUILTIN  -c ../Modules/_blake2/blake2module.c -o Modules/_blake2/blake2module.o
    2021-10-27T08:55:22.0563233Z Assembler messages:
    2021-10-27T08:55:22.0565823Z Fatal error: can't create Modules/_blake2/blake2module.o: No such file or directory
    2021-10-27T08:55:22.0622105Z Makefile:2177: recipe for target 'Modules/_blake2/blake2module.o' failed
    ...
    

    https://github.com/deadsnakes/python3.11-nightly/actions/runs/1389342824

    I believe this is due to https://github.com/python/cpython/pull/29225/files#diff-aac314cb71e95912e95795a4f4a278f2e75a5bc28e93f52e338185c8e7f7d7eaL225

    @brettcannon
    Copy link
    Member Author

    Could Brett or you please add those notes back ? There's no other place
    where such details are documented.

    It really depends on what "details" you're referring to. Most of what I removed were things like "Module by <person>", or saying _json.c is for "json accelerator" which is obvious to me. Anything that seemed pertinent to compilation I left in.

    So if there's something you specifically want to add back in that you think is important then please feel free to as I'm done editing the file for my purposes at the moment.

    @tiran
    Copy link
    Member

    tiran commented Oct 28, 2021

    New changeset 4c95fb4 by Christian Heimes in branch 'main':
    bpo-45548: Fix out-of-tree and Debian builds (GH-29263)
    4c95fb4

    @malemburg
    Copy link
    Member

    On 27.10.2021 22:58, Brett Cannon wrote:

    Brett Cannon <brett@python.org> added the comment:

    > Could Brett or you please add those notes back ? There's no other place
    where such details are documented.

    It really depends on what "details" you're referring to.

    I had already listed some of those details.

    Most of what I removed were things like "Module by <person>", or saying _json.c is for "json accelerator" which is obvious to me. Anything that seemed pertinent to compilation I left in.

    So if there's something you specifically want to add back in that you think is important then please feel free to as I'm done editing the file for my purposes at the moment.

    Will do.

    @tiran
    Copy link
    Member

    tiran commented Oct 29, 2021

    New changeset f0150ac by Christian Heimes in branch 'main':
    bpo-45548: Some test modules must be built as shared libs (GH-29268)
    f0150ac

    @brettcannon
    Copy link
    Member Author

    @christian are you using this issue for your pkg-config work, or should I close this?

    @tiran
    Copy link
    Member

    tiran commented Nov 23, 2021

    Erlend and I have split off the pkg-config and autoconf work into bpo-45847, bpo-45573, and others.

    @tiran tiran closed this as completed Nov 23, 2021
    @tiran tiran closed this as completed Nov 23, 2021
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.11 only security fixes build The build process and cross-build
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants