Implements requirements command as per #4959

[ImreC]

This implements a feature according to #4959.

[matteius]

@ImreC Could you merge latest main branch into your branch so the CI should pass again?

[ImreC]

@matteius is the news file correct? I have never worked with this before

[matteius]

@matteius is the news file correct? I have never worked with this before

It should be name 4959.feature.rst is the main thing, and anything that is code should be in double backticks. I have a linting PR out that will add pre-commit hooks to the project and so at that time it will be more obvious if you are naming the news fragments wrong among other things. I think the content itself makes sense, just remember to update the command name there as well.

[rittneje]

This should output the index verbatim as specified in the Pipfile, without expanding variables. It is fairly common to specify credentials for a private PyPI this way, and writing those into the resulting requirements.txt file is counter-productive.

I'm not sure whether your existing code is already doing this or not, just wanted to mention this need.

[oz123]

@ImreC thank you for the latest changes! really good to see this, just a small change still, and I think this can be merged.

[ImreC]

@oz123 I have one small question. I have been looking at the env var substitution (or rather lack thereof) as suggested by @rittneje and added a test for this, but something is going wrong on the shell/python intersection. The env var gets substituted in stdout where python checks, but I don't think it gets substituted anywhere else which is why the test fails. I expect this to not get substituted, but difficult to test in this way. I see possible paths forward:

• Know where this substitution takes place and prevent it somehow.
• Omit the test and assume that no env var gets expanded (which I think shouldn't happen)
  Any suggestions?

[ImreC]

For now I have changed the test back so it passes. All checks that I have performed indicates that this works as expected (the literal index URL including env var keys gets outputted with no expansion)

[matteius]

@ImreC I suspect for that test that shows the environment variables were not expanded, I think they are expanded. It has been in known issue reports, here are just some:
#3751
#3138

[ImreC]

@matteius but this could also explain the test failing. What my code does is that it takes the literal string in the package index and echoes that. I think that should work right? Another question that I have after looking at this is whether the code should support the case where a package index is linked to a package directly (as in requests = {index = "pypi",version = "*"}). Should this case be supported?

[matteius]

@ImreC I think it does explain your test failing and unless @rittneje has some inputs on this one, we may have to defer the environment variable in the pip indexes as a separate issue/enhancement.

For your second question, I am of the view that we should try to figure out how to support pinned indexes such as requests = {index = "pypi",version = "*"}). -- It is interesting though, because you are generating pip commands and I have patched pip in pipenv to be able to pin indexes correctly, meaning that the pip command is likely going to do it funky, potentially even wrong, by searching pypi and other indexes URLs for something that is index restricted.

[matteius]

Ok so requests = {index = "pypi",version = "*"}) isn't the best example because pypi is the default index and that basically gets pinned to pypi when when you leave off the index such as requests = {version = "*"}) ... a better example is torch:

[[source]]
url = "https://pypi.org/simple"
verify_ssl = true
name = "pypi"

[[source]]
url = "https://download.pytorch.org/whl/cu113/"
verify_ssl = true
name = "downloadpytorch"

[packages]
torch = {version = "==1.11.0+cu113", index = "downloadpytorch"}

[dev-packages]

[requires]
python_version = "3.10"

[ImreC]

@matteius I don't think adding a package index per line in requirements.txt is supported by pip, so I think this is impossible to achieve. The only way forward that I can see is to generate a requirements.txt file per package index, but this seems to deviate pretty far from the original gist of the feature: to be a simple way to generate a requirements.txt. Maybe we can just add a caveat to the readme and leave this as is? Potentially we can add this as an enhancement later, but I am also thinking that in the way it is implemented right now it would work for the majority of people. Thoughts?

[matteius]

@matteius I don't think adding a package index per line in requirements.txt is supported by pip, so I think this is impossible to achieve. The only way forward that I can see is to generate a requirements.txt file per package index, but this seems to deviate pretty far from the original gist of the feature: to be a simple way to generate a requirements.txt. Maybe we can just add a caveat to the readme and leave this as is? Potentially we can add this as an enhancement later, but I am also thinking that in the way it is implemented right now it would work for the majority of people. Thoughts?

@ImreC Yeah I agree with everything you have discovered and said about this. That was kind of my assumption too that pip did not support such a thing. Let's not make it a blocker to the fantastic work you've done on this to get it this far.

I do have one last thing for you to consider, and it doesn't have to be part of this PR -- but what do you think about the current option pipenv lock -r Should we drop support of however that is working in favor of this feature, or possibly if people are going to complain about removing the -r flag from lock, that we could make it use the same functionality as this requirements command. I haven't looked much yet at the implementation of the existing requirements.txt generation, but am curious if you have and what your thoughts are?

[ImreC]

I would definitely leave it in for now as the behavior is different than pipenv requirements, but it might make more sense to at some point take it out. Maybe pipenv lock does too much with the options currently in it. Conceptually it is maybe cleaner to use pipenv lock for creating a lockfile and doing that well, and then using pipenv requirements to generate requirements.txt from that, instead of trying to do everything from a single command with flags. I have not looked into the implementation, but happy to have a look at some point to see what it currently does.

[matteius]

@ImreC Looks like your build is failing on the new linting checks I added recently. Can you run pre-commit install on your branch and cleanup any errors generated during: pre-commit run --all-files --verbose. pip install pre-commit if you don't have it already.

[ImreC]

@matteius should be fixed

[oz123]

@matteius suppression expansion of variables is already merged into pip and I believe also into pipenv. So this should not happen here. Also, as @ImreC already mentioned index url are not part of requirements.txt.
I believe pip can handle the following:

$ cat requirements.txt
package_a==1.0.1 # from private pypi
package_b==2.0.2 # from another pypi
requests  # from pypi

can be resolved with:

pip install --extra-index-url https://private_a.org l --extra-index-url https://private_b.org

See also: https://stackoverflow.com/a/41926975

[oz123]

@ImreC you might want to change your author email. It seems currently wrong. If you are fine with the current email, I can merge it like this.

[ImreC]

@oz123 I changed my email address mid-PR so I think it is fine. Just for my understanding. Where is this visible?

[ImreC]

@matteius suppression expansion of variables is already merged into pip and I believe also into pipenv. So this should not happen here. Also, as @ImreC already mentioned index url are not part of requirements.txt. I believe pip can handle the following:

$ cat requirements.txt
package_a==1.0.1 # from private pypi
package_b==2.0.2 # from another pypi
requests  # from pypi

can be resolved with:

pip install --extra-index-url https://private_a.org l --extra-index-url https://private_b.org

See also: https://stackoverflow.com/a/41926975

@oz123 I think this case will work fine. The only problem that I can see is where you have packages with clashing names, f.e. you have a private package index with a package called requests in it. This will definitely not work with requirements.txt, even though I think pipenv offers the option to specifically define that this package needs to come from the private index and not pypi. Or maybe similarly named packages exist in multiple locations. We could solve this by defining multiple requirements.txt outputs, but it seems like a really exotic case to me. In case you control the private index a more obvious thing to do would be to not name your package requests, or add prefixes or something.

[matteius]

The only problem that I can see is where you have packages with clashing names, f.e. you have a private package index with a package called requests in it. This will definitely not work with requirements.txt, even though I think pipenv offers the option to specifically define that this package needs to come from the private index and not pypi.

@ImreC Actually this shouldn't be a big issue anymore, please see: #5029

TLDR: Basically all packages have to be pinned now to an index, and by default the first index in the Pipfile is the primary index that unspecified packages utilize.