Skip to content

[py] Moved Rust binary settings to pyproject.toml from setup.py#14837

Merged
VietND96 merged 4 commits intoSeleniumHQ:trunkfrom
sandeepsuryaprasad:setup
Dec 5, 2024
Merged

[py] Moved Rust binary settings to pyproject.toml from setup.py#14837
VietND96 merged 4 commits intoSeleniumHQ:trunkfrom
sandeepsuryaprasad:setup

Conversation

@sandeepsuryaprasad
Copy link
Contributor

@sandeepsuryaprasad sandeepsuryaprasad commented Nov 30, 2024 •
edited by qodo-code-review bot
Loading

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

    @qodo-code-review
    Copy link
    Contributor

    qodo-code-review bot commented Nov 30, 2024

    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

    @qodo-code-review
    Copy link
    Contributor

    qodo-code-review bot commented Nov 30, 2024

    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

    sandeepsuryaprasad commented Nov 30, 2024

    @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

    Dist package is https://test.pypi.org/project/selenium/4.28.0.202412041759/

    @VietND96 VietND96 merged commit 30cf1bc into SeleniumHQ:trunk Dec 5, 2024
    @sandeepsuryaprasad sandeepsuryaprasad mentioned this pull request Dec 5, 2024
    Closed
    8 tasks
    @jameshilliard jameshilliard mentioned this pull request Jul 19, 2025
    Merged
    jameshilliard
    jameshilliard reviewed Jul 19, 2025
    View reviewed changes
    py/pyproject.toml
    # include-package-data is `true` by default in pyproject.toml

    [[tool.setuptools-rust.bins]]
    target = "selenium.webdriver.common.selenium-manager"
    Copy link
    Contributor

    @jameshilliard jameshilliard Jul 19, 2025

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    This should have been:

    [[tool.setuptools-rust.ext-modules]]
target = "selenium.webdriver.common.selenium-manager"
binding = "Exec"

    Fixed in #16074.

    @dependabot dependabot bot mentioned this pull request Sep 18, 2025
    Merged
    @dependabot dependabot bot mentioned this pull request Oct 1, 2025
    Closed
    @dependabot dependabot bot mentioned this pull request Oct 24, 2025
    Closed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    @VietND96 VietND96 VietND96 approved these changes

    +1 more reviewer

    @jameshilliard jameshilliard jameshilliard left review comments

    Reviewers whose approvals may not affect merge requirements

    Assignees

    No one assigned

    Labels

    Review effort [1-5]: 1

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    3 participants

    @sandeepsuryaprasad @VietND96 @jameshilliard