Skip to content

pypi: include missing Grammar.txt and PatternGrammar.txt #1108

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

Closed
wants to merge 2 commits into from
Closed

pypi: include missing Grammar.txt and PatternGrammar.txt #1108

wants to merge 2 commits into from

Conversation

chenrui333
Copy link

@chenrui333 chenrui333 changed the title pypi: include missing third_party/yapf_third_party/_ylib2to3/Grammar.txt pypi: include missing Grammar.txt and PatternGrammar.txt Jun 14, 2023
@Spitfire1900
Copy link
Contributor

Spitfire1900 commented Jun 16, 2023

Running python -m setup sdist works on my machine, why is this required in MANIFEST.in here? Shouldn't

yapf/setup.py

Lines 87 to 94 in f4a7bb4

package_data={
'yapf_third_party': [
'yapf_diff/LICENSE',
'_ylib2to3/Grammar.txt',
'_ylib2to3/PatternGrammar.txt',
'_ylib2to3/LICENSE',
]
},
take care of this? Per https://packaging.python.org/en/latest/guides/using-manifest-in/#how-files-are-included-in-an-sdist package_data should be included.

Repro env:
python 3.10.6
setuptools==67.7.2
wheel==0.40.0

$ rm -rf dist yapf.egg-info & python -m setup sdist | grep -i GRAMMAR.txt
[1] 474414
warning: no files found matching '.travis.yml'
copying third_party/yapf_third_party/_ylib2to3/Grammar.txt -> yapf-0.40.0/third_party/yapf_third_party/_ylib2to3
copying third_party/yapf_third_party/_ylib2to3/PatternGrammar.txt -> yapf-0.40.0/third_party/yapf_third_party/_ylib2to3
[1]+  Done                    rm -rf dist yapf.egg-info

@Spitfire1900
Copy link
Contributor

@bwendling high priority triage for this PR.

Copy link
Contributor

@Spitfire1900 Spitfire1900 left a comment

Choose a reason for hiding this comment

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

Please include LICENSE files also.

Copy link
Contributor

@Spitfire1900 Spitfire1900 left a comment

Choose a reason for hiding this comment

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

Write tests to reproduce error condition.

I personally do not consider this a blocker for merging this PR, but it is a blocker for closing #1107

@char101
Copy link
Contributor

char101 commented Jun 18, 2023

Is this still apply https://stackoverflow.com/questions/7522250/how-to-include-package-data-with-setuptools-distutils ?

package_data is only applied to binary dist, in this case adding it in manifest.in seems the right path.

I tried running sdist and the txt files are included. I suspect the problem might be caused by stale yapf.egg-info\SOURCES.txt.

@chenrui333
Copy link
Author

side note, this is still needed for 0.40.1 release

@hartwork
Copy link
Contributor

side note, this is still needed for 0.40.1 release

Potentially not, please see comment #1107 (comment)

@hartwork
Copy link
Contributor

hartwork commented Jun 20, 2023

Maybe the real fix is adding setup_requires=['setuptools>=SOME_VERSION_HERE'] to the call to setuptools.setup in file setup.py?

@chenrui333
Copy link
Author

The two files were definitely missing in the 0.40.0 rel (you can untar this sdit file), but now got included in the 0.40.1 rel, let me check with my build in a bit :)

@hartwork
Copy link
Contributor

The two files were definitely missing in the 0.40.0 rel (you can untar this sdit file), but now got included in the 0.40.1 rel

@chenrui333 no doubt about that, I confirmed at #1107 (comment) in detail.

@chenrui333
Copy link
Author

Looks like we have sorted on this (the new version bump works fine on the homebrew side without the patch work), I am going to close this PR. Thanks @hartwork!

@chenrui333 chenrui333 closed this Jun 20, 2023
@chenrui333 chenrui333 deleted the add-missing-Grammar.txt branch June 20, 2023 17:06
@chenrui333
Copy link
Author

Please include LICENSE files also.

Now the two LICENSE files are all included in the 0.40.1 rel

+	third_party/yapf_third_party/_ylib2to3/Grammar.txt
+	third_party/yapf_third_party/_ylib2to3/LICENSE
+	third_party/yapf_third_party/_ylib2to3/PatternGrammar.txt
+	third_party/yapf_third_party/yapf_diff/LICENSE

@hartwork
Copy link
Contributor

Maybe the real fix is adding setup_requires=['setuptools>=SOME_VERSION_HERE'] to the call to setuptools.setup in file setup.py?

Went for that approach in #1115 now.

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

Successfully merging this pull request may close these issues.

PyPI package is missing Grammar.txt
4 participants