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] Fix installing most of the data from source distributions #15128

Open
wants to merge 3 commits into
base: trunk
Choose a base branch
from

Conversation

mgorny
Copy link

@mgorny mgorny commented Jan 21, 2025

User description

Description

Change namespaces to true to fix installing
selenium.webdriver.common.devtools.* packages, and fix package-data to fix installing .js and .json data files. The latter would specify a non-existing selenium_package package name -- instead, specify * to apply it to all Python packages found.

With these changes, the result of installing the source distribution is almost identical to the result of installing the wheel. One remaining problem is that selenium-manager is not included -- my guess is that there is no logic copying the built Rust executable into the expected location.

Another problem is that these changes do not work when installing from a VCS checkout -- since all the relevant data files are explicitly excluded by .gitignore and that takes precedence over data-files. However, this was broken before, so it should not really matter.

Motivation and Context

Fixes #15125 -- install from source distribution resulting in missing data files

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


Description

  • Enabled namespaces to fix package installation issues.

  • Updated package-data to include .js and .json files.

  • Resolved discrepancies between source distribution and wheel installation.

  • Addressed missing data files during source distribution installation.


Changes walkthrough 📝

Relevant files
Bug fix
pyproject.toml
Fix package and data file installation issues                       

py/pyproject.toml

  • Changed namespaces to true to fix package installation.
  • Updated package-data to apply to all Python packages.
  • Ensured inclusion of .py, .rst, .json, and .xpi files.
  • +2/-2     

    Need help?
  • Type /help how to ... in the comments thread for any question about Qodo Merge usage.
  • Check out the documentation for more information.
  • Change `namespaces` to true to fix installing
    `selenium.webdriver.common.devtools.*` packages, and fix `package-data`
    to fix installing `.js` and `.json` data files.  The latter would
    specify a non-existing `selenium_package` package name -- instead,
    specify `*` to apply it to all Python packages found.
    
    With these changes, the result of installing the source distribution is
    almost identical to the result of installing the wheel.  One remaining
    problem is that `selenium-manager` is not included -- my guess is that
    there is no logic copying the built Rust executable into the expected
    location.
    
    Another problem is that these changes do not work when installing from
    a VCS checkout -- since all the relevant data files are explicitly
    excluded by `.gitignore` and that takes precedence over `data-files`.
    However, this was broken before, so it should not really matter.
    
    Fixes SeleniumHQ#15125
    @CLAassistant
    Copy link

    CLAassistant commented Jan 21, 2025

    CLA assistant check
    All committers have signed the CLA.

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Package Data

    The wildcard "*" for package-data is very broad and might include unnecessary files. Consider being more specific about which packages need these data files.

    "*" = [
        "*.py",
        "*.rst",
        "*.json",
        "*.xpi",

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Fix missing array closing bracket

    Consider adding a closing bracket for the package-data array. The current
    configuration is missing the closing bracket for the array of file patterns.

    py/pyproject.toml [56-61]

     [tool.setuptools.package-data]
     "*" = [
         "*.py",
         "*.rst",
         "*.json",
         "*.xpi",
    +]
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: The suggestion correctly identifies a critical syntax error in the TOML file where the array is not properly closed, which could cause package configuration issues and build failures.

    9

    @mgorny
    Copy link
    Author

    mgorny commented Jan 21, 2025

    That linter seems to not "see" the whole file?

    @VietND96
    Copy link
    Member

    This is source dist is built from this PR https://test.pypi.org/project/selenium/4.29.0.202501211759/#files
    @mgorny, can you double check this before we release a patch?

    @mgorny
    Copy link
    Author

    mgorny commented Jan 21, 2025

    Yep, I can confirm it looks good (modulo what was said above):

     ----
     Name
    -selenium-4.28.0.dist-info/LICENSE
    -selenium-4.28.0.dist-info/METADATA
    -selenium-4.28.0.dist-info/RECORD
    -selenium-4.28.0.dist-info/WHEEL
    +selenium-4.29.0.202501211759.dist-info/LICENSE
    +selenium-4.29.0.202501211759.dist-info/METADATA
    +selenium-4.29.0.202501211759.dist-info/RECORD
    +selenium-4.29.0.202501211759.dist-info/top_level.txt
    +selenium-4.29.0.202501211759.dist-info/WHEEL
     selenium/common/exceptions.py
     selenium/common/__init__.py
     selenium/__init__.py
    @@ -265,9 +266,7 @@ selenium/webdriver/common/fedcm/dialog.p
     selenium/webdriver/common/fedcm/__init__.py
     selenium/webdriver/common/__init__.py
     selenium/webdriver/common/keys.py
    -selenium/webdriver/common/linux/selenium-manager
     selenium/webdriver/common/log.py
    -selenium/webdriver/common/macos/selenium-manager
     selenium/webdriver/common/mutation-listener.js
     selenium/webdriver/common/options.py
     selenium/webdriver/common/print_page_options.py
    @@ -278,7 +277,6 @@ selenium/webdriver/common/timeouts.py
     selenium/webdriver/common/utils.py
     selenium/webdriver/common/virtual_authenticator.py
     selenium/webdriver/common/window.py
    -selenium/webdriver/common/windows/selenium-manager.exe
     selenium/webdriver/edge/__init__.py
     selenium/webdriver/edge/options.py
     selenium/webdriver/edge/remote_connection.py

    @VietND96
    Copy link
    Member

    @diemol, the fix is verified and ready for patch 4.28.1

    @VietND96 VietND96 requested a review from diemol January 22, 2025 04:54
    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.

    [🐛 Bug]: 4.28.0 does not install Python package data when installing from source distribution
    3 participants