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
[WIP] Fix location of package prefixes #107
Conversation
5e5f2dc
to
9076c2c
Compare
Rebased on upstream/master |
❌ Build scikit-build 0.0.1.23 failed (commit d63f8dfff4 by @thewtex) |
✅ Build scikit-build 0.0.1.25 completed (commit cabf7c0d6b by @thewtex) |
Current coverage is 67.64% (diff: 80.64%)@@ master #107 diff @@
==========================================
Files 19 17 -2
Lines 488 510 +22
Methods 0 0
Messages 0 0
Branches 88 99 +11
==========================================
- Hits 347 345 -2
- Misses 104 125 +21
- Partials 37 40 +3
|
I will work on improving the coverage... |
Thanks. That would be nice. |
9076c2c
to
5d848f0
Compare
❌ Build scikit-build 0.0.1.56 failed (commit ddb971665c by @thewtex) |
7acd0de
to
8397a86
Compare
❌ Build scikit-build 0.0.1.57 failed (commit 3bff7234cf by @thewtex) |
✅ Build scikit-build 0.0.1.58 completed (commit 6cb595acaa by @thewtex) |
8397a86
to
5e3526d
Compare
✅ Build scikit-build 0.0.1.60 completed (commit 3df969b373 by @thewtex) |
5e3526d
to
c3e0822
Compare
✅ Build scikit-build 0.0.1.61 completed (commit df721a104f by @thewtex) |
@@ -194,7 +236,7 @@ def _classify_files(install_paths, package_data, package_prefixes, py_modules, | |||
" Project Root : {}\n" | |||
" Violating File: {}\n").format(install_root, test_path)) | |||
|
|||
# peel off the 'skbuild' prefix | |||
# peel off the 'skbuild' prefix |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be aligned with code on the next line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
The current implementation had logic errors -- it did not support paths containing a '.'. Support the "root package" and duplicate the logic from distutils, which is documented and expected. https://docs.python.org/2/distutils/setupscript.html#listing-whole-packages
c3e0822
to
7ed209c
Compare
👎 Please do not merge. Do possibly to a subtle change in setuptools (?), the wheel generated by the test no longer contains the C extension. Closing because I don't think it is worth putting more effort into this path. Focusing instead on #124 |
The current implementation had logic errors -- it did not support paths containing a
'.'.
Support the "root package" and duplicate the logic from distutils, which is
documented and expected.
https://docs.python.org/2/distutils/setupscript.html#listing-whole-packages
Note that this commit starts a new test directory,
unit
.