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

Minor: Undefined behavior leads to confusing error with --index-url arg. #7430

Closed
SalomonSmeke opened this issue Dec 4, 2019 · 6 comments · Fixed by #7965
Closed

Minor: Undefined behavior leads to confusing error with --index-url arg. #7430

SalomonSmeke opened this issue Dec 4, 2019 · 6 comments · Fixed by #7965
Labels
auto-locked Outdated issues that have been locked by automation state: awaiting PR Feature discussed, PR is needed type: enhancement Improvements to functionality

Comments

@SalomonSmeke
Copy link

SalomonSmeke commented Dec 4, 2019

Environment

  • pip version: 19.2.3
  • Python version: 3.8
  • OS: Arch Linux

I do not believe this is limited to arch, but it was discovered as such here: SalomonSmeke/rere#5

note: as pointed out by @uranusjr below this is partially fixed in 19.3

Description

A very minor problem, borderline not a bug but simply less than ideal behavior. When using a --index-url with pip install, passing something other than a url causes a confusing error.

This can manifest like in the issue above:
pip install --index-url --user https://test.pypi.org/simple/ reretrieve
when trying to:
pip install --user --index-url https://test.pypi.org/simple/ reretrieve

Expected behavior

An error about how the index url is invalid, not found, or otherwise messed up.

How to Reproduce

  1. pip install --index-url --user https://test.pypi.org/simple/ reretrieve

Output

$ pip install --index-url --user https://test.pypi.org/simple/ reretrieve
Looking in indexes: --user
Collecting https://test.pypi.org/simple/
  Using cached https://test.pypi.org/simple/
  ERROR: Cannot unpack file /tmp/pip-unpack-w9t0tixn/simple (downloaded from /tmp/pip-req-build-s5ub6ixa, content-type: text/html; charset=UTF-8); cannot detect archive format
ERROR: Cannot determine archive format of /tmp/pip-req-build-s5ub6ixa.

note: as pointed out by @uranusjr below in 19.3 the output will look a little different.

Edit

I just wanted to clarify that this happens with any bad text input/invalid url, not just --user and such.

$ pip install --index-url foo  https://test.pypi.org/simple/ reretrieve
Looking in indexes: foo
Collecting https://test.pypi.org/simple/
  Using cached https://test.pypi.org/simple/
  ERROR: Cannot unpack file /tmp/pip-unpack-160ive4r/simple (downloaded from /tmp/pip-req-build-8dw7kt5w, content-type: text/html; charset=UTF-8); cannot detect archive format
ERROR: Cannot determine archive format of /tmp/pip-req-build-8dw7kt5w

I dont blame you if you decide to wontfix this one, but wanted to make sure it was an intentional decision. thank you in advance for taking a look!

@triage-new-issues triage-new-issues bot added the S: needs triage Issues/PRs that need to be triaged label Dec 4, 2019
@uranusjr
Copy link
Member

uranusjr commented Dec 5, 2019

pip 19.3 has a warning like this:

WARNING: Url 'foo/pip/' is ignored. It is either a non-existing path or lacks a specific scheme.

that would probably help debug the problem. I kind of feel it can do a better job though; maybe detect the malformed index URL before it gets concatenated to the package name.

The cannot determine archive format error is much more complicated, unfortunately. pip allows specifying a package by URL, e.g.

pip install https://github.com/pypa/pip/archive/master.zip

and pip has no way to know whether you actually want to install https://test.pypi.org/simple/ as a package. So that part should probably be wontfix.

@SalomonSmeke
Copy link
Author

SalomonSmeke commented Dec 6, 2019

@uranusjr of course the next version from mine got the warning! That would have definitely saved me some time in debugging, but I agree that ideally the message is clearer.

cannot determine archive format is unimportant here. if something like:

$ pip install --index-url --user https://test.pypi.org/simple/ reretrieve
ERROR: index-url --user is invalid.
Collecting https://test.pypi.org/simple/
  Using cached https://test.pypi.org/simple/
  ERROR: Cannot unpack file /tmp/pip-unpack-w9t0tixn/simple (downloaded from /tmp/pip-req-build-s5ub6ixa, content-type: text/html; charset=UTF-8); cannot detect archive format
ERROR: Cannot determine archive format of /tmp/pip-req-build-s5ub6ixa.

happens, the true source of the problem would be immediately clear to me, or at worst just a lot less difficult to track down.

In fact, I would argue cannot determine archive format is accurate and should remain! Assuming pip would not rather just exit after ERROR: index-url --user is invalid..

@chrahunt chrahunt added this to the Print Better Error Messages milestone Dec 8, 2019
@chrahunt chrahunt added state: awaiting PR Feature discussed, PR is needed type: enhancement Improvements to functionality labels Dec 8, 2019
@triage-new-issues triage-new-issues bot removed the S: needs triage Issues/PRs that need to be triaged label Dec 8, 2019
@chrahunt
Copy link
Member

chrahunt commented Dec 8, 2019

I'm +1 on mentioning --index-url in the warning message, assuming it doesn't make a mess in the code.

People can have --index-url and --extra-index-url in requirements.txt which may point to directories that don't always exist, so I would be wary of turning this into a hard failure.

@deveshks
Copy link
Contributor

deveshks commented Apr 2, 2020

Hi @chrahunt , @uranusjr

Can I work on this PR? Also I was thinking about what would be the best place to check for such an issue.

I was thinking that we can insert an error in the below function, when we are checking the index urls. Or maybe there is a better place to put this error perhaps?

def get_formatted_locations(self):
# type: () -> str
lines = []
if self.index_urls and self.index_urls != [PyPI.simple_url]:
lines.append(
'Looking in indexes: {}'.format(', '.join(
redact_auth_from_url(url) for url in self.index_urls))
)

@uranusjr
Copy link
Member

uranusjr commented Apr 2, 2020

I’m not entirely sure, to be honest. I think part of the reason this is marked as awaiting PR is because it’s difficult to gauge the situation without diving into the implementation, and once you do that you’re in the best position to produce a PR. So I’d say go ahead writing some code first and we can see what we can work out.

@deveshks
Copy link
Contributor

deveshks commented Apr 2, 2020

Thanks @uranusjr , I have created a PR considering one possible approach. Please take a look :)

bors bot added a commit to duckinator/emanate that referenced this issue May 13, 2020
118: Update pip to 20.1 r=duckinator a=pyup-bot


This PR updates [pip](https://pypi.org/project/pip) from **20.0.2** to **20.1**.



<details>
  <summary>Changelog</summary>
  
  
   ### 20.1
   ```
   =================

Process
-------

- Document that pip 21.0 will drop support for Python 2.7.

Features
--------

- Add ``pip cache dir`` to show the cache directory. (`7350 &lt;https://github.com/pypa/pip/issues/7350&gt;`_)

Bug Fixes
---------

- Abort pip cache commands early when cache is disabled. (`8124 &lt;https://github.com/pypa/pip/issues/8124&gt;`_)
- Correctly set permissions on metadata files during wheel installation,
  to permit non-privileged users to read from system site-packages. (`8139 &lt;https://github.com/pypa/pip/issues/8139&gt;`_)
   ```
   
  
  
   ### 20.1b1
   ```
   ===================

Deprecations and Removals
-------------------------

- Remove emails from AUTHORS.txt to prevent usage for spamming, and only populate names in AUTHORS.txt at time of release (`5979 &lt;https://github.com/pypa/pip/issues/5979&gt;`_)
- Remove deprecated ``--skip-requirements-regex`` option. (`7297 &lt;https://github.com/pypa/pip/issues/7297&gt;`_)
- Building of local directories is now done in place, instead of a temporary
  location containing a copy of the directory tree. (`7555 &lt;https://github.com/pypa/pip/issues/7555&gt;`_)
- Remove unused ``tests/scripts/test_all_pip.py`` test script and the ``tests/scripts`` folder. (`7680 &lt;https://github.com/pypa/pip/issues/7680&gt;`_)

Features
--------

- pip now implements PEP 610, so ``pip freeze`` has better fidelity
  in presence of distributions installed from Direct URL requirements. (`609 &lt;https://github.com/pypa/pip/issues/609&gt;`_)
- Add ``pip cache`` command for inspecting/managing pip&#39;s wheel cache. (`6391 &lt;https://github.com/pypa/pip/issues/6391&gt;`_)
- Raise error if ``--user`` and ``--target`` are used together in ``pip install`` (`7249 &lt;https://github.com/pypa/pip/issues/7249&gt;`_)
- Significantly improve performance when ``--find-links`` points to a very large HTML page. (`7729 &lt;https://github.com/pypa/pip/issues/7729&gt;`_)
- Indicate when wheel building is skipped, due to lack of the ``wheel`` package. (`7768 &lt;https://github.com/pypa/pip/issues/7768&gt;`_)
- Change default behaviour to always cache responses from trusted-host source. (`7847 &lt;https://github.com/pypa/pip/issues/7847&gt;`_)
- An alpha version of a new resolver is available via ``--unstable-feature=resolver``. (`988 &lt;https://github.com/pypa/pip/issues/988&gt;`_)

Bug Fixes
---------

- Correctly freeze a VCS editable package when it is nested inside another VCS repository. (`3988 &lt;https://github.com/pypa/pip/issues/3988&gt;`_)
- Correctly handle ``%2F`` in URL parameters to avoid accidentally unescape them
  into ``/``. (`6446 &lt;https://github.com/pypa/pip/issues/6446&gt;`_)
- Reject VCS URLs with an empty revision. (`7402 &lt;https://github.com/pypa/pip/issues/7402&gt;`_)
- Warn when an invalid URL is passed with ``--index-url`` (`7430 &lt;https://github.com/pypa/pip/issues/7430&gt;`_)
- Use better mechanism for handling temporary files, when recording metadata
  about installed files (RECORD) and the installer (INSTALLER). (`7699 &lt;https://github.com/pypa/pip/issues/7699&gt;`_)
- Correctly detect global site-packages availability of virtual environments
  created by PyPA’s virtualenv&gt;=20.0. (`7718 &lt;https://github.com/pypa/pip/issues/7718&gt;`_)
- Remove current directory from ``sys.path`` when invoked as ``python -m pip &lt;command&gt;`` (`7731 &lt;https://github.com/pypa/pip/issues/7731&gt;`_)
- Stop failing uninstallation, when trying to remove non-existent files. (`7856 &lt;https://github.com/pypa/pip/issues/7856&gt;`_)
- Prevent an infinite recursion with ``pip wheel`` when ``$TMPDIR`` is within the source directory. (`7872 &lt;https://github.com/pypa/pip/issues/7872&gt;`_)
- Significantly speedup ``pip list --outdated`` by parallelizing index interaction. (`7962 &lt;https://github.com/pypa/pip/issues/7962&gt;`_)
- Improve Windows compatibility when detecting writability in folder. (`8013 &lt;https://github.com/pypa/pip/issues/8013&gt;`_)

Vendored Libraries
------------------

- Update semi-supported debundling script to reflect that appdirs is vendored.
- Add ResolveLib as a vendored dependency.
- Upgrade certifi to 2020.04.05.1
- Upgrade contextlib2 to 0.6.0.post1
- Upgrade distro to 1.5.0.
- Upgrade idna to 2.9.
- Upgrade msgpack to 1.0.0.
- Upgrade packaging to 20.3.
- Upgrade pep517 to 0.8.2.
- Upgrade pyparsing to 2.4.7.
- Remove pytoml as a vendored dependency.
- Upgrade requests to 2.23.0.
- Add toml as a vendored dependency.
- Upgrade urllib3 to 1.25.8.

Improved Documentation
----------------------

- Emphasize that VCS URLs using git, git+git and git+http are insecure due to
  lack of authentication and encryption (`1983 &lt;https://github.com/pypa/pip/issues/1983&gt;`_)
- Clarify the usage of --no-binary command. (`3191 &lt;https://github.com/pypa/pip/issues/3191&gt;`_)
- Clarify the usage of freeze command in the example of Using pip in your program (`7008 &lt;https://github.com/pypa/pip/issues/7008&gt;`_)
- Add a &quot;Copyright&quot; page. (`7767 &lt;https://github.com/pypa/pip/issues/7767&gt;`_)
- Added example of defining multiple values for options which support them (`7803 &lt;https://github.com/pypa/pip/issues/7803&gt;`_)
   ```
   
  
</details>


 

<details>
  <summary>Links</summary>
  
  - PyPI: https://pypi.org/project/pip
  - Changelog: https://pyup.io/changelogs/pip/
  - Homepage: https://pip.pypa.io/
</details>



Co-authored-by: pyup-bot <[email protected]>
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label May 20, 2020
@lock lock bot locked as resolved and limited conversation to collaborators May 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation state: awaiting PR Feature discussed, PR is needed type: enhancement Improvements to functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants