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

build: include .removed file in setup.py build #3653

Merged
merged 1 commit into from Apr 1, 2021

Conversation

amurzeau
Copy link
Contributor

This file is required by tests when executing them with the build dir as it is done in Debian (as tests might require native .so files to be compiled for some python packages for example).

This file is required by tests when executing them with the build dir.
@amurzeau amurzeau changed the title build: include .removed file in build build: include .removed file in setup.py build Mar 26, 2021
@mkbloke
Copy link
Member

mkbloke commented Mar 26, 2021

#3644 ?

@bastimeyer
Copy link
Member

This should already be covered by #3644, which added the file to MANIFEST.in. Hence the 2.1.1 release.

Changed in version 3.1: All the files that match package_data will be added to the MANIFEST file if no template is provided. See Specifying the files to distribute.

@amurzeau
Copy link
Contributor Author

MANIFEST.in seems to only affect sdist, but not build, I've done this becausse even with the 2.1.1 release, I get test errors (because tests are executed against streamlink files in the build directory).

@amurzeau
Copy link
Contributor Author

When doing python3 setup.py build, .removed is not copied to build/lib/streamlink/plugins/.removed unless adding the line in setup.py.

Copy link
Member

@bastimeyer bastimeyer left a comment

Choose a reason for hiding this comment

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

When doing python3 setup.py build, .removed is not copied to build/lib/streamlink/plugins/.removed unless adding the line in setup.py.

Hm, indeed, interesting...

Could you please also remove the entry from MANIFEST.in then, so that it's not specified twice?

@bastimeyer
Copy link
Member

Btw, not sure what makes more sense here, semantically speaking, package_data={"streamlink": ["plugins/.removed"]} or package_data={"streamlink.plugins": [".removed"]}, but I guess it doesn't really matter...

@amurzeau
Copy link
Contributor Author

I'm not 100% sure package_data will affect sdist as MANIFEST.in does, as as you, when I revert both MANIFEST.in and setup.py changes, I still get the .removed file in the sdist archive ... (I don't know why too).

Just found this: https://stackoverflow.com/a/14159430
It seems that package_data does not affect sdist, so the MANIFEST.in change is still required.

@amurzeau
Copy link
Contributor Author

The first comment of this stackoverflow answer says:

I have been researching this issue for the past hour and have been trying many approaches. As you say, package_data works for bdist and not sdist. However, MANIFEST.in works for sdist, but not for bdist! Therefore, the best I have been able to come up with is to include both package_data and MANIFEST.in in order to accommodate both bdist and sdist. – Wesley Baugh Mar 5 '13 at 0:41

Copy link
Member

@bastimeyer bastimeyer left a comment

Choose a reason for hiding this comment

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

Well, I guess it doesn't hurt leaving it in there. The file gets included on my system when I remove it from MANIFEST.in and keep the setup.py changes, but that's probably due to the "mysterious circumstances" which I described in #3644.

I was just wondering because of what I quoted above:
#3653 (comment)

This answer from stackoverflow kind of nails it:

Seriously, I feel like this ticket is a group therapy session for folks using setuptools and discovering just what a horrid place they have found themselves in life.


I won't merge this just yet, in case someone else has something to say.

You don't need another patch release after this, do you?

@amurzeau
Copy link
Contributor Author

No I don't need another patch release :-) I have this PR as a patch for this one, this is fine as-is. Merging this will let me remove it later when there will be a new release, keeping the distribution patches as low as possible, but this is not urgent.

@gravyboat
Copy link
Member

gravyboat commented Mar 27, 2021

I'm fine with this. The reasoning for it is sound as dumb as it is to have to include it. I'll leave this open just in case someone else comes along, otherwise I'll merge in a day or two.

@bastimeyer bastimeyer merged commit c7bd7ec into streamlink:master Apr 1, 2021
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