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

Fix logical error of generating MANIFEST. #260

Closed
wants to merge 1 commit into
base: master
from

Conversation

4 participants
@seanlis
Contributor

seanlis commented Jun 11, 2017

No description provided.

@codecov-io

This comment has been minimized.

codecov-io commented Jun 11, 2017

Codecov Report

Merging #260 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #260   +/-   ##
=======================================
  Coverage   91.05%   91.05%           
=======================================
  Files          26       26           
  Lines         939      939           
  Branches      159      159           
=======================================
  Hits          855      855           
  Misses         61       61           
  Partials       23       23
Impacted Files Coverage Δ
skbuild/command/generate_source_manifest.py 85.18% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b400676...3ab79f7. Read the comment docs.

@jcfr

This comment has been minimized.

Contributor

jcfr commented Jun 12, 2017

@seanlis Thanks for the patch 👍

Would you have the steps allowing to reproduce the problem ? Ideally, I would like to add a regression test capturing the problem fixed by our contribution.

Thanks a lot.
Jc

@seanlis

This comment has been minimized.

Contributor

seanlis commented Jun 12, 2017

  1. There is a MANIFEST.in in the project.
  2. There is no MANIFEST file.
  3. Run 'python setup.py sdist'
  4. A MANIFEST file would be generated by skbuild from the output of
    git ls-tree --name-only -r HEAD instead MANIFEST.in.
@jcfr

This comment has been minimized.

Contributor

jcfr commented Jun 15, 2017

@seanlis Thanks for the update.

Just to let you that I will be travelling for the next week and I am not sure if I will have time to follow up until I am back.

@henryiii

This comment has been minimized.

Contributor

henryiii commented Mar 12, 2018

Can this one-word change patch be applied? It's a critical bug that makes it impossible to use a MANIFEST.in, since if MANIFEST exists, MANIFEST.in will not write to it. I've manually made this patch to get the MANIFEST.in working before I found this PR.

It might be easier to work the logic through if you make the variable do_not_generate and then not it, since you're really checking for reasons not to try to generate the file.

To see if it works by only generating a MANIFEST file, use:

python setup.py sdist -o

Note, you have to delete your skbuild directory to get this working after making this change, since the marker file will be found otherwise.

And if you need a reason that a git based auto-manifest doesn't work; git submodules will not show up and fail to get included.

@henryiii henryiii referenced this pull request Mar 12, 2018

Merged

Fix Manifest bugs #310

@jcfr

This comment has been minimized.

Contributor

jcfr commented Apr 28, 2018

Closing. This is superseded by #310

@jcfr jcfr closed this Apr 28, 2018

jcfr added a commit that referenced this pull request May 11, 2018

jcfr added a commit that referenced this pull request May 11, 2018

Merge branch 'fixmanifest'
* fixmanifest:
  CHANGES: Add entry documenting fix for #260
  command/generate_source_manifest: Add test for MANIFEST.in
  command/generate_source_manifest: Fix logical error of generating MANIFEST.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment