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

[py] Remove precompiled binaries from sdist #14233

Merged
merged 1 commit into from
Jul 10, 2024

Conversation

jameshilliard
Copy link
Contributor

@jameshilliard jameshilliard commented Jul 8, 2024

User description

Python sdist's should not contain precompiled binaries that are architecture specific. They should contain the sources needed to build the binaries instead.

Description

This removes the precompiled binaries from the sdist and modifies the setuptools setup.py to build them with setuptools-rust from source.

For example in buildroot trying to compile selenium from the sdist for an arm target gives this error:

ERROR: architecture for "/usr/lib/python3.12/site-packages/selenium/webdriver/common/linux/selenium-manager" is "Advanced Micro Devices X86-64", should be "ARM"

Motivation and Context

When building python selenium from source we should not use any prebuilt binaries as they will often not be able to run on the target architecture. This is especially problematic when cross compiling.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

Bug fix, Enhancement


Description

  • Enhanced _get_binary function in selenium_manager.py to use the compiled path for selenium-manager if it exists.
  • Integrated setuptools-rust in setup.py to handle Rust extensions.
  • Updated Bazel build configuration to include Rust source files and exclude precompiled binaries.
  • Added pyproject.toml to define build system requirements for Rust integration.
  • Added file groups for Rust source files in rust/BUILD.bazel.

Changes walkthrough 📝

Relevant files
Enhancement
selenium_manager.py
Enhance `_get_binary` to use compiled path for `selenium-manager`

py/selenium/webdriver/common/selenium_manager.py

  • Added logic to determine the compiled path for selenium-manager.
  • Included sysconfig to get the executable suffix.
  • Updated _get_binary function to use the compiled path if it exists.
  • +8/-0     
    setup.py
    Integrate `setuptools-rust` for building Rust extensions 

    py/setup.py

  • Added setuptools-rust to handle Rust extensions.
  • Defined rust_extensions in setup configuration.
  • +7/-0     
    BUILD.bazel
    Update Bazel build to include Rust sources and exclude binaries

    py/BUILD.bazel

  • Included pyproject.toml in package sources.
  • Excluded precompiled binaries for different platforms.
  • Added Rust source files to the package.
  • +7/-0     
    BUILD.bazel
    Add file groups for Rust source files in Bazel build         

    rust/BUILD.bazel

    • Added file groups for Rust source files and dependencies.
    +15/-0   
    Configuration changes
    pyproject.toml
    Define build system requirements for Rust integration       

    py/pyproject.toml

  • Added build system requirements for setuptools and setuptools-rust.
  • Defined build-backend as setuptools.build_meta.
  • +3/-0     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Python sdist's should not contain precompiled architecture
    binaries that are architecture specific. They should contain
    the sources needed to build the binaries instead.
    Copy link
    Contributor

    qodo-merge-pro bot commented Jul 8, 2024

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Possible Bug:
    The code modification in selenium_manager.py introduces a new path construction for the selenium-manager binary using sysconfig.get_config_var("EXE"). This change assumes that the EXE configuration variable is always set, which might not be the case in all environments. This could potentially lead to issues where the compiled_path does not have the correct executable suffix, causing runtime errors when the path is used.

    Dependency Management:
    The PR adds setuptools-rust as a dependency in pyproject.toml and modifies setup.py to include Rust extensions. It's crucial to ensure that these changes are compatible with the existing Python setup and do not introduce conflicts with other parts of the project that might rely on the Python-only setup.

    Copy link
    Contributor

    qodo-merge-pro bot commented Jul 8, 2024

    PR Code Suggestions ✨

    No code suggestions found for PR.

    @diemol
    Copy link
    Member

    diemol commented Jul 8, 2024

    How would we distribute the Selenium Manager binaries?

    Note that our build tool is Bazel.

    @jameshilliard
    Copy link
    Contributor Author

    How would we distribute the Selenium Manager binaries?

    The wheel build still uses the Selenium Manager binaries. The sdist is not supposed to have architecture specific precompiled binaries AFAIU.

    @jameshilliard
    Copy link
    Contributor Author

    Hmm, tests apparently failing for //py:common-chrome-bidi-test/selenium/webdriver/common/bidi_tests.py but I'm unable to reproduce a failure locally and it doesn't appear to be related to these changes.

    @AutomatedTester
    Copy link
    Member

    Hmm, tests apparently failing for //py:common-chrome-bidi-test/selenium/webdriver/common/bidi_tests.py but I'm unable to reproduce a failure locally and it doesn't appear to be related to these changes.

    I think this is a flaky test. I am not concerned

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    3 participants