An almost working implementation to install from .whl #3
Conversation
| temp = path.with_name("{}.tmp.{}".format(path.name, self._name)) | ||
| with temp.open(**kwargs) as f: | ||
| yield f | ||
| temp.replace(path) |
There was a problem hiding this comment.
given it also replaces I feel like just open is not adequate naming here
There was a problem hiding this comment.
then again what's the point of this tmp file rather than just direct-write?
There was a problem hiding this comment.
I stole this pattern from pip and thought the original implementation must exist for a reason. Likely some kind of edge case about writing to an already-open file.
I’m not really in love with the naming of this either, but can’t think of a better one.
There was a problem hiding this comment.
@pradyunsg care to enlighten us? for me at the moment seems just a needless complication 😊 At least let's document it why we need this awkward way.
There was a problem hiding this comment.
I tried to trace this, and… it takes me straight back to the initial implementation of wheel support! cc @dholth
There was a problem hiding this comment.
It's likely to prevent concurrency issues, the rename operation being atomic.
There was a problem hiding this comment.
If so I consider this a bad approach, it silently swallows an important error. Sure you will not throw a failure... but instead, you're getting an environment that might not be what you expect. I'd rather throw the failure to notify the user. E.g. imagine two processes install in parallel different versions; with this approach, we'll not throw an error, but the created version will be a combination of version A and B. I'd rather throw and let the user know that something bad is happening.
There was a problem hiding this comment.
Not to mention that while you might not get failure on the .dist-info file, you can still get the same concurrency failure on .dist-info.tmp file.
| installer.install(options.dest) | ||
|
|
||
|
|
||
| if __name__ == "__main__": |
There was a problem hiding this comment.
yeah this should go into the __main__ 👍
|
|
||
|
|
||
| class InvalidWheel(Exception): | ||
| pass |
There was a problem hiding this comment.
instead of pass consider adding a docstring that explains when the error may be thrown
What does this entail? Is this
Either of those sounds reasonable to me. |
Yup. |
|
I’m going to separate this and submit them as individual PRs instead. This contains at least two entirely different concerns. |
I implemented the whole workflow to identify what are the missing pieces. And it’s almost working!
$ py examples/whl.py <..whl> <directory> [--installer <installer-name>](See pypa/pip#8156 for background of the installer name feature.)
There are two areas I have not implemented:
copyreference parts of distlib.direct_url.json). I’m not sure about how we should implement this. pip already has an implementation, but I don’t feel like copying it directly since we only need a small subset of it (to write the file, not read and make sense of it). Maybe we should put it somewhere instead, e.g.packagingorimportlib-metadata? The installer can just accept an optionalDirectURLinstance and work with that.And then we can start identifying features we can make sans-IO. I already pulled out a part of making sense of the wheel structure (where the
.dist-infodirectory is, and format various files in it). Some other potential parts I can think of:RECORD). Especially if we have PEP 610 support, since that is more than writing a string to a file.