Support relative paths and regularize path handling and formatting #1650
Conversation
AndydeCleyre
force-pushed
the
feature/relpaths-post-6.8.0
branch
2 times, most recently
from
0bdf7a7
to
07a2948
Compare
AndydeCleyre
force-pushed
the
feature/relpaths-post-6.8.0
branch
from
07a2948
to
3f9518a
Compare
|
I think the Windows-only testing is not properly updating the coverage report. |
|
@AndydeCleyre, the lines that are showing as not covered anymore after this PR in Codecov report are hit only on Python 3.11. There is only a single job running under Python 3.11 and it failed to upload the coverage to Codecov. Thank you for implementing this change! |
AndydeCleyre
force-pushed
the
feature/relpaths-post-6.8.0
branch
from
3f9518a
to
8a43d1f
Compare
|
I'm just pushing periodic rebases, but I'm not making any changes. |
If passed, it will attempt to relative-ize the resulting URL (if possible). Add helper: fragment_string
Tests: - abs_ireq_preserves_source_ireqs - format_requirement_impossible_relative_path_becomes_absolute (Windows-only) Also, ensure copy_install_requirement preserves our custom attributes.
Specifically ones which start like file:///C:/
Ensure source ireqs' comes_from attr is credited in annotations
This brings with it more involved string manipulation
If passed, from_dir will be used instead of CWD to interpret relative paths. Special care is taken to guarantee _was_relative in the yielded ireqs, if originally relative. When calling format_requirement, _was_relative's presence now determines what's passed as from_dir: either CWD or None (None leads to an absolute path output) For ths to work we also re-assign the _was_relative attr to ireqs generated from candidates in the backtracking resolver.
With test, and needed changes: - Use this flag to decide what to pass as from_dir to parse_requirements - Add init param to OutputWriter: write_relative_to_output - Add corresponding from_dir logic and use to OutputWriter._format_requirement - Add test_write_relative_to_output which runs with the new flag both on and off
This ensures absolute intact paths for git+file: reqs
With test_sync_relative_path If the reqs in the output were written relative to that output file (pip-compile --write-relative-to-output), then it's wise for pip-sync to interpret reqs it finds in there from the location of the output file. This is now possible with pip-sync --read-relative-to-input. To be clear: pip-sync's "input" is pip-compile's "output."
This fixes at least two problems, for which tests are added.
And add tests via parameterization
e.g. ../../rawr#egg=rawr-cli
This fixes false-positive _was_relative assignments for some direct references like: ptrender[yaml] @ file:///home/andy/Code/ptrender
An ireq's extras may be any Collection, not necessarily a set, so we can't rely on set operators working implicity.
As we always want to set that True
AndydeCleyre
force-pushed
the
feature/relpaths-post-6.8.0
branch
from
8a43d1f
to
aa3ee0f
Compare
|
It seems the coverage upload fails very regularly. I wonder if anyone has interest in pursuing a change like this? |
I don't think so, I keep using Codecov, and it worked well in the past years. There are bugs, of course, that get fixed (some — by me: https://github.com/codecov/uploader/issues?q=author%3Awebknjaz). But in any case, I'd first try to get to the root of the issue you're seeing before making such radical changes which may not improve things. That said, it's clear that some jobs may have failed to upload coverage. It is currently hard to easily find out which ones. So I'd strongly recommend to:
re:3 — I've looked at https://github.com/jazzband/pip-tools/actions/runs/3175334504/jobs/5173249582#step:11:37 and was confused by it complaining about the actions/checkout action. So I scrolled up and, to my horror, saw it using the |
I noticed recently that Dependabot can now update your actions for you: https://docs.github.com/en/code-security/dependabot/working-with-dependabot/keeping-your-actions-up-to-date-with-dependabot |
|
Please don't disable codecov, it fails because is primatly because it is not properly tuned. There are solutions for each of its problems. For example, making it require a number of success uploads bit lower than total. For example 9/10, or 8/10. Making it prevent failing a build if upload fails. Setting a rounding threshold to prevent it from posting a regression due to rounding errors. It is essential to be able to browse the diff with its web interface in order to allow us to spot if some branches are not covered, especially for bigger changes. Also, I would not keep its report as "required" for merge. I think we can all look at it an make a decision if we are about to ignore it or not. |
Yes, for as long as you're pointing at a tag or a commit sha but not branch. FWIW I've already made a PR updating this stuff. Along with a few others. |
It was never required, just red. |
|
FYI these GHA improvements are awaiting approvals: https://github.com/jazzband/pip-tools/pulls?q=is%3Apr+is%3Aopen+author%3Awebknjaz+sort%3Aupdated-desc |
There was a problem hiding this comment.
I've tried to review this many times, but every time I caught myself thinking, that these changes are too hacky and bring too much complexity to the codebase.
As far as I remember the initial intention was to preserve -e . in requirements.txt, but then everything started growing as a snowball at some point - annotations, pip-sync, new options and etc.
format_reqirements() now thrice bigger because of the from_dir and looks like it breaks principles of single responsibility.
parse_requirements() looks too hacky due to changing workdir and parsing requirements with catching InstallationError and parsing again without fragments.
Parsing fragments using regexps also no good.
Sorry, I cannot approve this for the reason in the first paragraph. I know how much energy you've invested into it and much appreciated it. I'd want to step back and rethink this PR. Probably split this into smaller chunks, I don't know, whether it would help. I'd rather not have this feature rather than maintain unreliable code.
|
Closing for the reasons mentioned by @atugushev - Please do not take it personally. I do not disagree with original idea but proposed implementation does touch too much code and adds undesired complexity. Maybe we could look into achieving this gradually with small/atomic changes, so we can slowly move the code-base towards the desired destination. Keep in mind that there are also other changes coming, so small changes will be less likely to conflict. |
|
Thank you for giving it as much chance as you did. All I can say is that I did my best given the constraints of backward compatibility and making no changes to pip itself. |
|
I would like to see something like this happen, as it causes extraneous diffs when updating requirements.txt files in automation that runs in temp directories. I'm currently working around it by patching over the generated files in my automation. @ssbarnea any chance you could recommend how to break this up into smaller chunks? I totally feel where you're coming from and would be willing to help work on those but I don't know the codebase well enough to know how to split it up. |
|
FWIW these 29 commits are such that all tests pass at each step along the way, though not always fully covered by new tests at each step. |
This replaces #1329, rebased with care on top of pip-tools 6.8.0.
Changelog-friendly one-liners:
file:,<vcs>+file:, or..pip-compilewill interpret relative paths in an input file as relative to that input file, rather than the current folder, if--read-relative-to-inputis passed.pip-compilewill reconstruct relative req paths as relative to the output file, rather than the current folder, if--write-relative-to-outputis passed.pip-syncwill interpret relative paths in an input file as relative to that input file, rather than the current folder, if--read-relative-to-inputis passed.pkg[extra1,extra2].-ewith a relative local path, is only installable from a specific directory pypa/pip#6112Contributor checklist
Maintainer checklist
backwards incompatible,feature,enhancement,deprecation,bug,dependency,docsorskip-changelogas they determine changelog listing.