Skip to content
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

Updating remote links with new URLs for PEP508 functionallity #5780

Open
cam72cam opened this issue Sep 13, 2018 · 51 comments · May be fixed by #10564
Open

Updating remote links with new URLs for PEP508 functionallity #5780

cam72cam opened this issue Sep 13, 2018 · 51 comments · May be fixed by #10564
Assignees
Labels
C: direct url Direct URL references (PEP 440, PEP 508, PEP 610) PEP implementation Involves some PEP state: awaiting PR Feature discussed, PR is needed type: enhancement Improvements to functionality

Comments

@cam72cam
Copy link

What's the problem this feature will solve?
Installing a wheel file from a URL will not install a new wheel if the link changes.

Example:
setup.py

install_requires=[
    "package@http://domain:port/link-0.1.3.whl
]

If you change that link and try to re-install or upgrade the parent package, pip ignores the new requirement.

If there is no way for the url/version to be checked when installing dependencies, the URL support added in PEP 508 seems problematic at best.

Describe the solution you'd like
Installing remote links should always be reinstalled, same as editable links seems to be the simplest method.

Alternative Solutions
We could instead try to guess/determine/specify versions in the requirement string.
Ex:

"package=0.1.3@http://domain:port/link-0.1.3.whl"

which would imply that the link provides version 0.1.3 that we could check against the currently installed package version

@pfmoore
Copy link
Member

pfmoore commented Sep 13, 2018

What's the use case for the link changing? In particular, what's the situation where you'd have:

  1. The URL changes
  2. The project name and version remain the same
  3. The code is different

I'm particularly uncomfortable with assuming that the code could be different even thought the name/version is the same, so I'd like to see a strong justification for why that's the case (the assumption that if the name and version are the same, the built project will be, is pretty pervasive throughout the pip codebase).

@cam72cam
Copy link
Author

The problem is the name is the same, but the version changes.

If I change "package@http://domain:port/link-0.1.3.whl" to "package@http://domain:port/link-0.2.5.whl", it won't install the new version of the package. When I re-install or upgrade the current package.

The package@remote notation will never install a newer version of the specified package as it's written right now.

@benoit-pierre
Copy link
Member

The problem is PEP 508 direct URLs don't allow for version specifiers (otherwise you'd just be able to update the version, like for other requirements), so the only way pip would know the package version changed is to "prepare" it.

@cam72cam
Copy link
Author

Exactly

@benoit-pierre
Copy link
Member

Frankly, they really are a poor substitute for dependency links.

@cam72cam
Copy link
Author

Something I noticed is that pip install functions a bit different. It looks like it compares the remote URL

pip install http://server:port/path/package-0.1.whl
(installs package-0.1)
pip install http://server:port/path/package-0.1.whl
(already installed message)
pip install http://server:port/path/package-0.2.whl
(installs new version)
pip install http://server:port/path/package-0.2.whl
(already installed message)
pip install http://server:port/different/path/package-0.2.whl
(already installed message)

@benoit-pierre
Copy link
Member

AFAIK, the requirement is prepared, but not installed, because the version is already present.

@benoit-pierre
Copy link
Member

How about something along those lines:

 src/pip/_internal/req/constructors.py | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git i/src/pip/_internal/req/constructors.py w/src/pip/_internal/req/constructors.py
index 4c4641dc..50292ab8 100644
--- i/src/pip/_internal/req/constructors.py
+++ w/src/pip/_internal/req/constructors.py
@@ -176,7 +176,7 @@ def install_req_from_editable(
 
 def install_req_from_line(
     name, comes_from=None, isolated=False, options=None, wheel_cache=None,
-    constraint=False
+    constraint=False, package_name=None,
 ):
     """Creates an InstallRequirement from a name, which might be a
     requirement, directory containing 'setup.py', filename, or URL.
@@ -263,6 +263,8 @@ def install_req_from_line(
                 "Invalid requirement: '%s'\n%s" % (req, add_msg)
             )
 
+    assert package_name is None or req.name == package_name
+
     return InstallRequirement(
         req, comes_from, link=link, markers=markers,
         isolated=isolated,
@@ -292,6 +294,11 @@ def install_req_from_req(
             "which are not also hosted on PyPI.\n"
             "%s depends on %s " % (comes_from.name, req)
         )
+    if req.url:
+        return install_req_from_line(req.url, comes_from=comes_from,
+                                     isolated=isolated,
+                                     wheel_cache=wheel_cache,
+                                     package_name=req.name)
 
     return InstallRequirement(
         req, comes_from, isolated=isolated, wheel_cache=wheel_cache

So basically package @ URL is treated like URL.

@cam72cam
Copy link
Author

That works perfectly for my test cases. I'll close my PR if you can open one from that patch.

@pfmoore
Copy link
Member

pfmoore commented Sep 13, 2018

OK, cool. Thanks for clarifying.

I prefer the alternative proposal of determining the version from the filename in the URL, if possible, when parsing a direct URL, just like we do for a dependency picked up of PyPI (getting the version from a .whl filename is well-specified, and we have standard heuristics for doing so from sdists). That sounds like a far better solution than just always installing direct URLs,

@benoit-pierre
Copy link
Member

It still needs some work (particularly with respect to extras handling), but I'll see if I can make a proper PR.

cam72cam added a commit to cam72cam/pip that referenced this issue Sep 13, 2018
@cam72cam
Copy link
Author

@benoit-pierre do you think it would be possible to add this to the 18.1 milestone?

@cam72cam cam72cam changed the title Dependencies with remote links should be automatically reinstalled Updating remote links with new URLs for PEP508 functionallity Sep 24, 2018
@pradyunsg pradyunsg modified the milestone: 18.1 Sep 24, 2018
@pradyunsg
Copy link
Member

I added this to 18.1, but it seems like there's additional discussion + work wanted here. I doubt this would be ready in time for 18.1, which is due early next month.

@benoit-pierre
Copy link
Member

I'm waiting for #5788 to be merged. My intent was then to PR this change: package@url_to/package-0.6-py3-none-any.whl would be handled like url_to/package-0.6-py3-none-any.whl on the command line (in other instances, using package@url would still be equivalent to using url#egg=package on the command line).

But really, I've come to the conclusion that the lack of version specifier when using direct URLs is crippling, they certainly are no replacement for dependency links.

@pradyunsg
Copy link
Member

Ping? What's the next step here?

@rouge8
Copy link
Contributor

rouge8 commented Mar 6, 2019

Is there anything that can be done about this? pip not reinstalling PEP 508 dependencies when the URL changes makes that feature much less useful 😕

@cam72cam
Copy link
Author

cam72cam commented Mar 8, 2019

I've been using a patched pip from #5780 (comment)

@rouge8
Copy link
Contributor

rouge8 commented Apr 12, 2019

I opened a PR (#6402) based on the patch above (#5780 (comment)), with tests

@pradyunsg
Copy link
Member

@uranusjr Are you going to be picking this up for this release, or do you wanna kick the can down the road for this?

@pradyunsg pradyunsg added the S: awaiting response Waiting for a response/more information label Sep 21, 2021
@uranusjr
Copy link
Member

If we’re targeting a feature freeze in early October then I won’t hav enough time to finish this. Feel free to reassign this to 22.0.

@pradyunsg pradyunsg modified the milestones: 21.3, 22.0 Sep 22, 2021
@pradyunsg
Copy link
Member

Feel free to reassign this to 22.0.

Done!

@no-response
Copy link

no-response bot commented Oct 6, 2021

This issue has been automatically closed because there has been no response to our request for more information from the original author. With only the information that is currently in the issue, we don't have enough information to take action. Please reach out if you have or find the answers we need so that we can investigate further.

@no-response no-response bot closed this as completed Oct 6, 2021
@chief1983
Copy link

This issue has been automatically closed because there has been no response to our request for more information from the original author. With only the information that is currently in the issue, we don't have enough information to take action. Please reach out if you have or find the answers we need so that we can investigate further.

Then can I at least get clarification on whether the issue I brought up should be filed as a separate issue or is intended behavior? No one has actually responded to it but apparently a few others thought it was a noteworthy change to address.

@andersk
Copy link
Contributor

andersk commented Oct 6, 2021

This seems like either an incorrect application of, or an incorrect automated response to, the “S: awaiting response” label. There is no missing information in the report. The information that was requested when “S: awaiting response” was added (a) was requested of another developer, not of the original author, and (b) was immediately provided. This should be reopened.

@chief1983
Copy link

Yeah I wasn't sure but this seemed a bit premature to me as well.

@pradyunsg
Copy link
Member

Dropped this from 22.0, based on the discussion over at #10564.

jtpavlock added a commit to MoeMusic/Moe that referenced this issue Oct 12, 2022
It seems installing via the git method doesn't actually do anything differently. Potentially related issue:

pypa/pip#5780
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: direct url Direct URL references (PEP 440, PEP 508, PEP 610) PEP implementation Involves some PEP state: awaiting PR Feature discussed, PR is needed type: enhancement Improvements to functionality
Projects
None yet