-
Notifications
You must be signed in to change notification settings - Fork 2
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
Implement explicit lockfiles integration #23
Conversation
This is how the output of
We can't print the URL for PyPI wheels because it wasn't kept around 😬 |
I'm not personally a fan of using the CLI flags here but these are the problems I faced trying to obtain something else:
I wish there was a way to encode the constraints in a single string field (like a URI or something), but right now this is the only way, I think. We can document this as the official syntax... which happens to be compatible with a subset of the pip CLI 😬 Note we are adding more flags to the command. |
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.
A few notes:
- I'm not sure how I feel you expanding this format without a spec and a CEP since that's definitely a core functionality in conda
- There has to be other ways than implementing custom argv parsing and custom Python packaging standards parsing (big maintenance risk based on my experience)
- Inline docs and high-level reasoning for some of the design decisions are sorely needed before merging this
conda_pypi/main.py
Outdated
if path.name == "WHEEL" and path.parent.suffix == ".dist-info": | ||
for line in path.read_text().splitlines(): | ||
line = line.strip() | ||
if ":" not in line: | ||
continue | ||
key, value = line.split(":", 1) | ||
if key == "Tag": | ||
wheel.setdefault(key, []).append(value.strip()) | ||
else: | ||
wheel[key] = value.strip() |
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.
Isn't there a standard way to get this info without manual parsing?
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.
I dug in the docs and it uses RFC email headers, so email.parser.HeaderParser
from stdlib can be used. It requires a bit of post-processing, quite similar to what we see in conda.common.pkg_formats.python
. That module would better be here in this project now. That whole thing is mostly there to support conda list
listing pypi
dependencies after all. We can refactor it out and finally remove the pip_interop_enabled
thingy.
if TYPE_CHECKING: | ||
from typing import Iterable | ||
|
||
log = getLogger(f"conda.{__name__}") |
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.
Interesting, is there a reason to reuse the conda logger instead of an own?
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.
I've been doing this in all my plugins to reuse conda
's configuration for consistency. CLS does this too, for example.
conda_pypi/main.py
Outdated
"--report", | ||
json_output.name, | ||
"--target", | ||
json_output.name + ".dir", |
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.
why .dir?
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.
We need a writable path here. I'm just reusing the tmpfile. I don't know if the path is actually created, though. This is a dry run after all.
conda_pypi/main.py
Outdated
|
||
|
||
def compute_record_sum(record_path, algo): | ||
record = Path(record_path).read_text() |
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.
OSError again
conda_pypi/main.py
Outdated
# we only want to check the metadata and wheel parts of dist-info; everything else | ||
# is not deterministic or useful | ||
continue | ||
if path.parts[0] == ".." and any(part in path.parts for part in ("bin", "lib", "Scripts")): |
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.
if path.parts[0] == ".." and any(part in path.parts for part in ("bin", "lib", "Scripts")): | |
elif path.parts[0] == ".." and any(part in path.parts for part in ("bin", "lib", "Scripts")): |
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.
A few notes:
- I'm not sure how I feel you expanding this format without a spec and a CEP since that's definitely a core functionality in conda
- There has to be other ways than implementing custom argv parsing and custom Python packaging standards parsing (big maintenance risk based on my experience)
- Inline docs and high-level reasoning for some of the design decisions are sorely needed before merging this
Thanks for the thorough review!
I do want the CEP. For the original format too. There's no standard to refer to other than the implementation. It should be straightforward.
Yep, just wanted to get some external confirmation that this idea is worth pursuing before writing details on something that might not fly. I take that this review is also a 👍 to proceed further? |
There you go: conda/ceps#79 |
Hey @jezdez, I think I addressed most of your comments now. The PR is getting a bit too big so we can address further changes in new PRs. WDYT? After this PR I feel comfortable moving things to |
@jaimergp thank you for doing the rounds on the standardization, this'll be a net benefit! |
Closes #21
conda list --explicit
extensionsconda install | create
extensions for the above output