The Wayback Machine - https://web.archive.org/web/20211108071411/https://github.com/pytorch/pytorch/pull/52908
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

Jump to bottom

Add MANIFEST.in #52908

Closed
wants to merge 7 commits into from
Closed

Add MANIFEST.in #52908

wants to merge 7 commits into from

Conversation

Reviewers

@xuzhao9 xuzhao9

@mattip mattip

@facebook-github-bot facebook-github-bot

@rgommers rgommers

@seemethere seemethere

@walterddr walterddr

Assignees
No one assigned
Labels
cla signed Merged
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

setup.py sdist does not include third party submodules
7 participants
@malfet @facebook-github-bot @mattip @rgommers @xuzhao9 @seemethere @walterddr
@malfet
Copy link
Contributor

@malfet malfet commented Feb 26, 2021 •
edited
Loading

Do not build PyTorch if setup.py is called with 'sdist' option
Regenerate bundled license while sdist package is being built
Refactor check_submodules out of build_deps and check that submodules project are present during source package build stage.

Test that sdist package is configurable during asan-build step

Fixes #52843
@malfet
Add MANIFEST.in
6945164 
Also, do not build PyTorch when 'sdist' option is called
@facebook-github-bot
Copy link
Contributor

@facebook-github-bot facebook-github-bot commented Feb 26, 2021 •
edited
Loading

💊 CI failures summary and remediations

As of commit 4781447 (more details on the Dr. CI page):

💚 💚 Looks good so far! There are no failures yet. 💚 💚

This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Loading

facebook-github-bot
facebook-github-bot reviewed Feb 26, 2021
View changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

@malfet has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Loading

@malfet malfet requested review from mattip and Feb 26, 2021
@mattip
Copy link
Collaborator

@mattip mattip commented Feb 26, 2021

In order to correctly concatentate the two license files ./LICENSE and ./third_party/LICENSES_BUNDLED.txt, the sdist command should be wrapped like the class wheel_concatenate(bdist_wheel) class.

Loading

rgommers
rgommers reviewed Feb 26, 2021
View changes
MANIFEST.in Outdated Show resolved Hide resolved
Loading
@codecov
Copy link

@codecov codecov bot commented Feb 26, 2021

Codecov Report

Merging #52908 (6945164) into master (f40c9db) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #52908      +/-   ##
==========================================
- Coverage   80.76%   80.76%   -0.01%     
==========================================
  Files        1975     1975              
  Lines      216701   216701              
==========================================
- Hits       175029   175015      -14     
- Misses      41672    41686      +14

Loading

xuzhao9
xuzhao9 reviewed Feb 26, 2021
View changes
MANIFEST.in Outdated Show resolved Hide resolved
Loading
malfet added 2 commits Feb 27, 2021
@malfet
Partially Incorporate review feedback
1ed3c3e 
Add MANIFEST to .gitignore files
Add few more extensions to ignore/pruning list in MANIFEST.in
Update bundled licenses during sdist installation
Extract `check_submodules` routine from `build_deps`
@malfet
Test that sdist package is configurable
add0f75
facebook-github-bot
facebook-github-bot reviewed Feb 27, 2021
View changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

@malfet has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Loading

mattip
mattip reviewed Feb 28, 2021
View changes
.jenkins/pytorch/build.sh Outdated Show resolved Hide resolved
Loading
@mattip
Copy link
Collaborator

@mattip mattip commented Feb 28, 2021

I expected to see some traces of the build in the pytorch_linux_xenial_py3_clang5_asan_build, but it seems that job calls exec .. build-asan.sh and exits without continuing. Is there another *asan* build that hits the new test?

Loading

malfet added 2 commits Mar 1, 2021
@malfet
Fix typo
36ca511
@malfet
Applied @mattip suggestion
38acada
malfet
malfet reviewed Mar 1, 2021
View changes
setup.py Outdated Show resolved Hide resolved
Loading
@malfet
Move test sdist logic from build.sh to build-asan.sh
620c296
@malfet
Copy link
Contributor Author

@malfet malfet commented Mar 1, 2021

I expected to see some traces of the build in the pytorch_linux_xenial_py3_clang5_asan_build, but it seems that job calls exec .. build-asan.sh and exits without continuing. Is there another *asan* build that hits the new test?

Thank you for spotting. Moved the test logic from build.sh to build-asan.sh

Loading

@malfet malfet force-pushed the malfet/add-MANIFEST.in branch from 99f0577 to 620c296 Mar 1, 2021
facebook-github-bot
facebook-github-bot reviewed Mar 1, 2021
View changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

@malfet has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Loading

@malfet
Slightly change test script
4781447
facebook-github-bot
facebook-github-bot reviewed Mar 2, 2021
View changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

@malfet has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Loading

@facebook-github-bot
Copy link
Contributor

@facebook-github-bot facebook-github-bot commented Mar 2, 2021

@malfet merged this pull request in 272dfc7.

Loading

@mattip
Copy link
Collaborator

@mattip mattip commented Mar 2, 2021

Cool.
The build log has a warning: sdist: standard file not found: should have one of README, README.txt at the beginning of python setup.py sdist, and lots and lots of lines about creating the sdist.

Then at the beginning of the cmake-only build (python setup.py build --cmake-only) there is a fatal: Not a git repository (or any of the parent directories): .git but it is not really fatal. Other than that, the cmake warnings seem to be the usual ones I see when building from a git checkout.

Loading

@mattip mattip mentioned this pull request Mar 2, 2021
Open
@malfet malfet deleted the malfet/add-MANIFEST.in branch Mar 2, 2021
@mattip mattip mentioned this pull request Mar 5, 2021
Closed
aocsa added a commit to Quansight/pytorch that referenced this issue Mar 15, 2021
@malfet @aocsa
Add MANIFEST.in (pytorch#52908)
834fac1 
Summary:
Do not build PyTorch if `setup.py` is called with  'sdist' option
Regenerate bundled license while sdist package is being built
Refactor `check_submodules` out of `build_deps` and check that submodules project are present during source package build stage.

Test that sdist package is configurable during `asan-build` step

Fixes pytorch#52843

Pull Request resolved: pytorch#52908

Reviewed By: walterddr

Differential Revision: D26685176

Pulled By: malfet

fbshipit-source-id: 972a40ae36e194c0b4e0fc31c5e1af1e7a815185
xsacha pushed a commit to xsacha/pytorch that referenced this issue Mar 31, 2021
@malfet Sacha Refshauge
Add MANIFEST.in (pytorch#52908)
0f2c064 
Summary:
Do not build PyTorch if `setup.py` is called with  'sdist' option
Regenerate bundled license while sdist package is being built
Refactor `check_submodules` out of `build_deps` and check that submodules project are present during source package build stage.

Test that sdist package is configurable during `asan-build` step

Fixes pytorch#52843

Pull Request resolved: pytorch#52908

Reviewed By: walterddr

Differential Revision: D26685176

Pulled By: malfet

fbshipit-source-id: 972a40ae36e194c0b4e0fc31c5e1af1e7a815185
@malfet malfet mentioned this pull request Aug 5, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment