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

Remove package_data and data_files options from setup.py #5982

Merged
merged 1 commit into from Jun 26, 2018
Merged

Remove package_data and data_files options from setup.py #5982

merged 1 commit into from Jun 26, 2018

Conversation

wiggin15
Copy link
Contributor

@wiggin15 wiggin15 commented Jun 3, 2018

With include_package_data option set to True in setup.py, package data that is specified in MANIFEST.in is installed into the target directory when installing selenium. This means that we don't need to specify package_data and data_files in setup.py too. With the options set as before, files were included twice in wheel distributions (once in their original place and once in the ".data" directory) and that triggered a bug that caused setuptools to fail to install the wheel distribution of selenium.

I verified that running python setup.py bdist_wheel and python setup.py sdist create distributions with the data files inside and that the data files are installed when installing the created distributions.

Fixes issue #5907

With `include_package_data` option set to `True` in `setup.py`, package data that is specified in MANIFEST.in is installed into the target directory when installing selenium. This means that we don't need to specify `package_data` and `data_files` in setup.py too. With the options set as before, files were included twice in wheel distributions (once in their original place and once in the ".data" directory) and that triggered a bug that caused setuptools to fail to install the wheel distribution of selenium.
Copy link
Contributor

@davehunt davehunt left a comment

Choose a reason for hiding this comment

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

This looks okay, and makes sense. I'm concerned that there may have been a reason for including these keys in setup.py, but it's not one that I recall. The tests mostly pass for me locally, and I'm not convinced the failures are related to this change. Unfortunately there's an issue preventing the tests from running on Travis CI, which #5990 may assist with.

@p0deje p0deje added the C-py label Jun 7, 2018
@lmtierney
Copy link
Member

Ok, I've finally had time to try this on a Windows machine and it looks like it's fine. We'll get it into the next release and if it produces problems we'll be able to immediately do a point release.

@lmtierney lmtierney merged commit d9de47c into SeleniumHQ:master Jun 26, 2018
@lmtierney
Copy link
Member

Thank you for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants