Skip to content

Add selenium-manager executables to python manifest#13998

Merged
diemol merged 2 commits intoSeleniumHQ:trunkfrom
yuzawa-san:selenium-manager-python-manifest
May 21, 2024
Merged

Add selenium-manager executables to python manifest#13998
diemol merged 2 commits intoSeleniumHQ:trunkfrom
yuzawa-san:selenium-manager-python-manifest

Conversation

@yuzawa-san
Copy link
Contributor

@yuzawa-san yuzawa-san commented May 21, 2024 •
edited by qodo-code-review bot
Loading

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 •
    edited
    Loading

    CLA assistant check
    All committers have signed the CLA.

    @qodo-code-review qodo-code-review bot added P-enhancement PR with a new feature P-bug fix PR addresses a known issue Review effort [1-5]: 1 labels May 21, 2024
    @qodo-code-review
    Copy link
    Contributor

    qodo-code-review bot commented May 21, 2024

    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
    @qodo-code-review
    Copy link
    Contributor

    qodo-code-review bot commented May 21, 2024

    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

    diemol
    diemol approved these changes May 21, 2024
    View reviewed changes
    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!

    @diemol diemol merged commit 978fe21 into SeleniumHQ:trunk May 21, 2024
    @seedysoft seedysoft mentioned this pull request Jun 21, 2024
    Merged
    @maresb maresb mentioned this pull request Jun 22, 2024
    Closed
    5 tasks
    @MiguelPelegrina MiguelPelegrina mentioned this pull request Jul 12, 2024
    Merged
    @aravindb26 aravindb26 mentioned this pull request Jul 16, 2024
    Open
    @x-way x-way mentioned this pull request Jul 24, 2024
    Closed
    @x-way x-way mentioned this pull request Aug 10, 2024
    Closed
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Oct 29, 2024
    @yuzawa-san @diemol @sandeepsuryaprasad
    Add selenium-manager executables to python manifest (SeleniumHQ#13998)
    e9758cc 
    Add selenium-manager artifacts to python manifest

Co-authored-by: Diego Molina <diemol@users.noreply.github.com>
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    @diemol diemol diemol approved these changes

    Assignees

    No one assigned

    Labels

    P-bug fix PR addresses a known issue P-enhancement PR with a new feature Review effort [1-5]: 1

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    3 participants

    @yuzawa-san @CLAassistant @diemol