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

Add selenium-manager executables to python manifest #13998

Merged
merged 2 commits into from
May 21, 2024

Conversation

yuzawa-san
Copy link
Contributor

@yuzawa-san yuzawa-san commented May 21, 2024

User description

Description

Add the selenium-manager executables to the MANIFEST.in file so it gets installed correctly.

Motivation and Context

The python sdist installation was not copying the selenium-manager executables when doing a pip install /path/to/sdist. This error was happening:

selenium.common.exceptions.WebDriverException: Message: Unable to obtain working Selenium Manager binary; /tmp/testvirtualenv/lib/python3.12/site-packages/selenium/webdriver/common/macos/selenium-manager

This is not an issue in the wheel install since the bazel build automatically includes those.
However, I am forced to use the sdist (via the homebrew-pypi-poet).

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

  • Added selenium-manager executables for Linux, macOS, and Windows to the MANIFEST.in file to ensure they are included in the Python sdist installation.
  • This change fixes an issue where the selenium-manager executables were not being copied during pip install /path/to/sdist, causing a WebDriverException.
  • This issue did not affect wheel installations, as the bazel build includes these executables automatically.

Changes walkthrough 📝

Relevant files
Enhancement
MANIFEST.in
Include `selenium-manager` executables in Python sdist     

py/MANIFEST.in

  • Added selenium-manager executables for Linux, macOS, and Windows to
    the MANIFEST.in file.
  • +3/-0     

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

    @CLAassistant
    Copy link

    CLAassistant commented May 21, 2024

    CLA assistant check
    All committers have signed the CLA.

    Copy link
    Contributor

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    1, because the changes are straightforward and limited to the addition of three lines in the MANIFEST.in file to include necessary executables for different operating systems. This is a simple update with a clear purpose.

    🧪 Relevant tests

    No

    ⚡ Possible issues

    No

    🔒 Security concerns

    No

    @yuzawa-san yuzawa-san changed the title Add selenium-manager artifacts to python manifest Add selenium-manager executables to python manifest May 21, 2024
    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Maintainability
    Use wildcard patterns to include all selenium-manager files in the manifest

    To ensure that all necessary files are included, consider using a wildcard pattern for the
    selenium-manager files. This will make the manifest more maintainable and reduce the risk
    of missing files in the future.

    py/MANIFEST.in [18-20]

    -include selenium/webdriver/common/linux/selenium-manager
    -include selenium/webdriver/common/macos/selenium-manager
    -include selenium/webdriver/common/windows/selenium-manager.exe
    +include selenium/webdriver/common/linux/selenium-manager*
    +include selenium/webdriver/common/macos/selenium-manager*
    +include selenium/webdriver/common/windows/selenium-manager*.exe
     
    Suggestion importance[1-10]: 7

    Why: The suggestion to use wildcard patterns for including selenium-manager files is beneficial for maintainability and reduces the risk of missing future file variations. This is a practical improvement, though not critical.

    7

    Copy link
    Member

    @diemol diemol left a comment

    Choose a reason for hiding this comment

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

    Thank you, @yuzawa-san!

    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