python / cpython

The Python programming language
https://www.python.org
Other
63.62k stars 30.48k forks source link

Make zlib required on all platforms (simplifies code) #91246

Open gpshead opened 2 years ago

gpshead commented 2 years ago
BPO 47090
Nosy @gpshead
PRs
  • python/cpython#32043
  • 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: ```python assignee = 'https://github.com/gpshead' closed_at = None created_at = labels = ['extension-modules', 'build', 'library', '3.11'] title = 'Make zlib required on all platforms (simplifies code)' updated_at = user = 'https://github.com/gpshead' ``` bugs.python.org fields: ```python activity = actor = 'gregory.p.smith' assignee = 'gregory.p.smith' closed = False closed_date = None closer = None components = ['Build', 'Extension Modules', 'Library (Lib)'] creation = creator = 'gregory.p.smith' dependencies = [] files = [] hgrepos = [] issue_num = 47090 keywords = ['patch'] message_count = 2.0 messages = ['415750', '415809'] nosy_count = 1.0 nosy_names = ['gregory.p.smith'] pr_nums = ['32043'] priority = 'normal' resolution = None stage = 'patch review' status = 'open' superseder = None type = None url = 'https://bugs.python.org/issue47090' versions = ['Python 3.11'] ```

    gpshead commented 2 years ago

    We have a pile of conditionals and extra code in CPython to deal with building on systems that do not have zlib. The zlib C library has been around forever at this point and should be present on every system in the world.

    zlib is already required on Windows to build CPython.

    Proposal: simplify our code by removing the conditionals around zlib being optional. I'm attaching a draft PR to show what this would look like.

    gpshead commented 2 years ago

    Bringing this up on discord, others point out that the windows build requires zlib for convenience when we transitioned from having a vendored copy in our repo and that smaller "embedded" use cases may not like this if they don't already need the dep. But there have been no complaints about it on the Windows side.

    binascii.crc32 was one thing that motivated me to look into "just require zlib" to get out of carrying our own suboptimal crc32 code.

    looking over my PR, it can make for some awkward code with zlib right next to others that we treat as optionals. good bad or indifferent? i'm leaning towards indifferent and still enjoying fewer lines of code.

    gpshead commented 1 year ago

    discussion started in https://discuss.python.org/t/lets-make-zlib-required-rather-than-optional-to-build-cpython/23062

    katharosada commented 1 year ago

    @brettcannon asked me to update here.

    I've been experimenting with zlib and WASI cpython - if the zlib source is available, then it successfully builds with the WASI SDK and it pretty much just works - the cpython configure & build process detects it, it builds successfully and the zlib module seems to work fine when imported (also passes test_zlib). No code changes needed to either zlib or cpython.

    I built zlib with the WASI SDK and these environment variables for configure:

    prefix=${WASI_SDK_PATH}/share/wasi-sysroot \
    libdir=${WASI_SDK_PATH}/share/wasi-sysroot/lib/wasm32-wasi \
    pkgconfigdir=${WASI_SDK_PATH}/share/wasi-sysroot/lib/pkgconfig \
    CC="${WASI_SDK_PATH}/bin/clang --sysroot=${WASI_SDK_PATH}/share/wasi-sysroot" \
    ./configure

    Then normal make and make install. I presume it could be installed to a different place if necessary and included but I haven't tried that.

    Let me know if there's anything you'd like me to change/test, happy to write up PRs etc.

    Full Dockerfile for context:

    FROM ubuntu:latest
    
    ENV WASI_SDK_PATH /opt/wasi-sdk
    ENV WASI_SDK_VERSION 20
    
    RUN apt-get update && apt-get install -y curl
    
    RUN  mkdir ${WASI_SDK_PATH} && \
        curl -s -S --location https://github.com/WebAssembly/wasi-sdk/releases/download/wasi-sdk-${WASI_SDK_VERSION}/wasi-sdk-${WASI_SDK_VERSION}.0-linux.tar.gz | \
        tar --strip-components 1 --directory ${WASI_SDK_PATH} --extract --gunzip
    
    # Check
    RUN $WASI_SDK_PATH/bin/clang --version
    
    RUN apt-get install -y build-essential pkg-config \
        libbz2-dev libffi-dev libgdbm-dev libgdbm-compat-dev \
        liblzma-dev libsqlite3-dev libssl-dev \
        lzma lzma-dev uuid-dev python3 python-is-python3 zip
    
    # Install wasmtime and add to PATH
    RUN curl https://wasmtime.dev/install.sh -sSf | bash
    ENV PATH ${PATH}:/root/.wasmtime/bin
    
    # Set up PATH to have WASI SDK compile tools (clang, ar, strip etc.) taking precedence
    ENV PATH ${WASI_SDK_PATH}/bin/:${PATH}
    
    ENV ZLIB_DIR /zlib
    ENV ZLIB_VERSION 1.2.13
    RUN mkdir ${ZLIB_DIR} && curl https://www.zlib.net/zlib-${ZLIB_VERSION}.tar.gz | tar --directory ${ZLIB_DIR} --extract --gunzip
    
    RUN cd ${ZLIB_DIR}/zlib-${ZLIB_VERSION} && \
        prefix=${WASI_SDK_PATH}/share/wasi-sysroot libdir=${WASI_SDK_PATH}/share/wasi-sysroot/lib/wasm32-wasi pkgconfigdir=${WASI_SDK_PATH}/share/wasi-sysroot/lib/pkgconfig CC="${WASI_SDK_PATH}/bin/clang --sysroot=${WASI_SDK_PATH}/share/wasi-sysroot" ./configure && \
        make && make install
    
    # Fetch cpython source code
    RUN mkdir /cpython
    WORKDIR /cpython
    RUN curl https://github.com/python/cpython/archive/refs/heads/main.zip -L -o cpython.zip && \
        unzip cpython.zip
    
    WORKDIR /cpython/cpython-main
    
    # RUN python3 ./Tools/wasm/wasm_build.py wasi build
    # RUN python3 ./Tools/wasm/wasm_build.py wasi test
    # RUN python3 ./Tools/wasm/wasm_build.py wasi repl