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

BLD: fix include list for sdist building #13086

Merged
merged 1 commit into from Mar 6, 2019
Merged

Conversation

rgommers
Copy link
Member

@rgommers rgommers commented Mar 4, 2019

Closes gh-11927

To verify the changes I ran python setup.py sdist on master and on the branch this PR was made from, unpacked each sdist and ran this script:

import os


def get_filelist(root):
    allfiles = []
    for path, subdirs, files in os.walk(root):
        for name in files:
            allfiles.append(os.path.join(path, name)[len(root):])

    return allfiles


path1 = 'numpy_master/numpy-1.17.0.dev0+48b0f3c/'
path2 = 'numpy_manifest/numpy-1.17.0.dev0+a6bc29d/'

files_master = get_filelist(path1)
files_new = get_filelist(path2)

print("Files now in sdist that weren't included previously:")
for name in files_new:
    if not name in files_master:
        print(name)

print("\n\nFiles now left out:")
for name in files_master:
    if not name in files_new:
        print(name)

The result:

Files now in sdist that weren't included previously:
cythonize.dat
numpy/linalg/lapack_lite/fortran.py
numpy/linalg/lapack_lite/f2c_d_lapack.f.patch
numpy/linalg/lapack_lite/clapack_scrub.py
numpy/linalg/lapack_lite/LICENSE.txt
numpy/linalg/lapack_lite/f2c_s_lapack.f.patch
numpy/linalg/lapack_lite/f2c_config.c.patch
numpy/linalg/lapack_lite/f2c_z_lapack.f.patch
numpy/linalg/lapack_lite/README.rst
numpy/linalg/lapack_lite/f2c_lapack.f.patch
numpy/linalg/lapack_lite/make_lite.py
numpy/linalg/lapack_lite/f2c_c_lapack.f.patch
numpy/linalg/lapack_lite/wrapped_routines


Files now left out:

I think those should all be included.

The LICENSE.txt file was the important one pointed out in the bug report. The missing cythonize.dat was also not good, that must have triggered recompilation of Cython files for source builds that we didn't intend have happen.

@eric-wieser
Copy link
Member

I'm not sure any of the lapack_lite files need to be included. They represent a manual build step that is performed prior to committing, and are not something anyone else should either be running or depending upon.

@rgommers
Copy link
Member Author

rgommers commented Mar 4, 2019

I'm not sure any of the lapack_lite files need to be included. They represent a manual build step that is performed prior to committing, and are not something anyone else should either be running or depending upon.

I think you're thinking that they're not needed for installation, hence don't package? I'm thinking of the sdist more as an archive of the tagged source tree (the first purpose in https://www.python.org/dev/peps/pep-0517/#terminology-and-goals). We also package benchmarks, docs and tools, and I don't see a very strong reason to leave out only these few files.

@rgommers rgommers closed this Mar 4, 2019
@rgommers rgommers reopened this Mar 4, 2019
@rgommers
Copy link
Member Author

rgommers commented Mar 4, 2019

Hmm, the travisci build complains: warning: no files found matching 'cythonize.dat'

@mattip
Copy link
Member

mattip commented Mar 4, 2019

"cythonize.dat" is a build artifact containing the hashes of the target files used to trigger a cythonize run when they change.

@rgommers
Copy link
Member Author

rgommers commented Mar 4, 2019

"cythonize.dat" is a build artifact containing the hashes of the target files used to trigger a cythonize run when they change.

Yes I know - we should be including it to avoid users who build from source having to recompile the Cython files. I think it fell out of the sdist by accident, but I'm not sure yet - will investigate.

@rgommers
Copy link
Member Author

rgommers commented Mar 5, 2019

I think it fell out of the sdist by accident, but I'm not sure yet - will investigate.

Okay, we avoid the need for that because we only call generate_cython from within this if-statement: if not os.path.exists(os.path.join(cwd, 'PKG-INFO')):. So never for sdists. Removed cythonize.dat again, since setuptools is being weird about not finding it (locally that works fine for me, probably some setuptools behavior change).

@rgommers rgommers added this to the 1.17.0 release milestone Mar 5, 2019
@rgommers
Copy link
Member Author

rgommers commented Mar 5, 2019

Okay all green, should be good to merge

@mattip mattip merged commit efff9d8 into numpy:master Mar 6, 2019
@mattip
Copy link
Member

mattip commented Mar 6, 2019

Thanks @rgommers

@charris charris added the 09 - Backport-Candidate PRs tagged should be backported label Mar 6, 2019
@charris charris modified the milestones: 1.17.0 release, 1.16.3 release Mar 6, 2019
@rgommers rgommers deleted the manifest branch March 6, 2019 17:17
@charris charris removed the 09 - Backport-Candidate PRs tagged should be backported label Mar 16, 2019
@charris charris removed this from the 1.16.3 release milestone Mar 16, 2019
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.

None yet

4 participants