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] Moved Rust binary settings to pyproject.toml from setup.py #14837

Merged
merged 4 commits into from
Dec 5, 2024

Conversation

sandeepsuryaprasad
Copy link
Contributor

@sandeepsuryaprasad sandeepsuryaprasad commented Nov 30, 2024

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

Moved Rust binary settings to pyproject.toml from setup.py thus completely eliminating the need of setup.py

Motivation and Context

  • This PR completely eliminates the need of setup.py as all the settings are moved to pyproject.toml
  • After the change is made, I ran the build on my local machine bazel build //py:selenium-wheel which is successful.
  • Installed selenium from .whl file, the installation was successful.
  • Verified that selenium-manager is present in selenium.webdriver.common folder (in /linux, /macos, /windows)
  • Verified the installation with --no-binary option as well.

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

enhancement


Description

  • Removed the setup.py file, migrating its configurations to pyproject.toml.
  • Updated pyproject.toml to include Rust binary settings for selenium-manager.
  • Modified Bazel build configuration by removing setup.py from the source files.

Changes walkthrough 📝

Relevant files
Enhancement
setup.py
Remove setup.py and migrate configurations                             

py/setup.py

  • Removed the setup.py file entirely.
  • Eliminated the need for setup.py by moving configurations.
  • +0/-38   
    pyproject.toml
    Add Rust binary settings to pyproject.toml                             

    py/pyproject.toml

    • Added Rust binary settings for selenium-manager.
    +3/-0     
    Configuration changes
    BUILD.bazel
    Update Bazel build configuration for setup.py removal       

    py/BUILD.bazel

    • Removed setup.py from the list of source files.
    +0/-1     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Configuration Validation
    Verify that the Rust binary target path 'selenium.webdriver.common.selenium-manager' matches the expected installation location and naming convention

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    Replace hyphens with underscores in Python package paths to follow standard naming conventions

    The Rust binary target path should use dots instead of hyphens to maintain Python
    package naming conventions and avoid potential import issues.

    py/pyproject.toml [47-48]

     [[tool.setuptools-rust.bins]]
    -target = "selenium.webdriver.common.selenium-manager"
    +target = "selenium.webdriver.common.selenium_manager"
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Using hyphens in Python package paths can cause import issues as hyphens are not valid in Python identifiers. Replacing with underscores follows Python naming conventions and prevents potential runtime errors.

    8

    💡 Need additional feedback ? start a PR chat

    @sandeepsuryaprasad
    Copy link
    Contributor Author

    @VietND96 Can you please build this one for me from CI and upload to test PyPI. So that i will verify the binaries..

    @VietND96
    Copy link
    Member

    VietND96 commented Dec 2, 2024

    I think other script need to be updated, since publish CI failed https://github.com/SeleniumHQ/selenium/actions/runs/12113072764/job/33767482214
    Will support you once I have time.

    @VietND96
    Copy link
    Member

    VietND96 commented Dec 4, 2024

    @VietND96 VietND96 merged commit 30cf1bc into SeleniumHQ:trunk Dec 5, 2024
    43 checks passed
    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.

    2 participants