Improve the install cache with symlinks method #822
Conversation
Codecov Report
@@ Coverage Diff @@
## main #822 +/- ##
==========================================
+ Coverage 83.34% 83.64% +0.29%
==========================================
Files 74 74
Lines 6305 6347 +42
Branches 1132 1147 +15
==========================================
+ Hits 5255 5309 +54
+ Misses 751 722 -29
- Partials 299 316 +17
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| # Otherwise, the directory is likely a namespace package, | ||
| # mkdir and create links for all files inside. |
There was a problem hiding this comment.
Very nice! From glancing at the code, it seems you support nested namespace packages (symlinking files only)?
I also see that you remove previous symlinks if they exist: I think it means the order in which you iterate on files matters. I believe CPython usually just sorts such dirs and files list when searching for a module/package, so in our case we'd need to reverse-sort since the last processed file wins?
There was a problem hiding this comment.
Yeah, but I guess it won't work if the namespace packages are nested in more than 2 levels:
a # <- this will be detected as normal directory and create link for it
b
c
__init__.py
But I think it is enough.
There was a problem hiding this comment.
Ah I see. Too bad, I actually have a doubly nested namespace package 😅
But it's OK, pth files work fine, and I'm planning to change that to a level-1 namespace package later anyway.
Thanks!
There was a problem hiding this comment.
it is easy for a recursive detection, but i am not sure if a deeply nested package can still be called a 'package'(think of a/b/c/d/e.py and there is no py file under any level of parents). However, if you agree every py files inside a wheel must belong to a package, the answer would be Yes.
There was a problem hiding this comment.
Well, that's how PEP420 packages work, so I'd say yes, with a/b/c/d/e.py, a is a (namespace) package (and b, c, and d are sub-namespace packages, or namespace sub-packages haha).
| for child in Path(root).iterdir(): | ||
| if ( | ||
| child.is_file() | ||
| and child.suffix in (".py", ".pyc", ".pyo", ".pyd") |
There was a problem hiding this comment.
I wonder if .so files should be checked as well.
There was a problem hiding this comment.
I've thought of that, but it was not that easy to tell a .so is a python extension module or not. So this case can be ignored given how rare it is to put a .so inside a PEP 420 namespace package.
There was a problem hiding this comment.
I've just encountered that case 😂 That's actually why I'm commenting this 😂
But yeah, more than the extension, the whole file name would need to be checked against some regular expression like ^(?P<module_name>\w+)\.(?P<build_info>.+)\.so$ (from my limited knowledge)😕
But you're certainly right and this is unnecessary for PDM (at least for now).
Pull Request Check List
news/describing what is new.Describe what you have changed in this PR.
Close #820