4.2 re-orders requirements.txt before installing, breaking extra pypi index

[vickeryj]

We have some packages in a secondary pypi server so our requirements.txt looks something like:

-i https://pypi.org/simple
--extra-index-url https://repo.fury.io/techadmin/
amazon-kclpy==1.5.0
argh==0.26.2
...

A recent change this package to pip.js seems to re-order this:
https://github.com/UnitedIncome/serverless-python-requirements/blob/master/lib/pip.js#L287

to

--extra-index-url https://repo.fury.io/techadmin/
-i https://pypi.org/simple
amazon-kclpy==1.5.0
argh==0.26.2

Now, this works fine with pip install, but when serverless tries to install with this file it can't find anything in the repo.fury.io index.

[dschep]

Thanks for the bug report @vickeryj! @AndrewFarley, any chance you could take a look at this since you implemented the majority of the changes that make up 4.2.0?

[AndrewFarley]

@dschep Absolutely, I totally can see the bug. I know exactly what will do it. Give me a few minutes to get setup on my laptop and I'll get you a MR shortly.

[AndrewFarley]

@vickeryj Can you please try my MR?

rm -Rf node_modules/serverless-python-requirements
npm i github:andrewfarley/serverless-python-requirements#bug-fix-requirements-ordering

Please note: you will need to change your requirements.txt file to be...

--index-url https://pypi.org/simple
--extra-index-url https://repo.fury.io/techadmin/
amazon-kclpy==1.5.0
argh==0.26.2
...

So, as far as I can tell, the requirement.txt format guide says that all internal options should be prefixed with -- and only at the start of the file, because all "normal" lines (packages to install) may optionally use single line prefix switches (eg -e). I think this is why pip and your config is getting confused. I'm using this logic to "skip" re-ordering any lines that are prefixed with --. In fact, with the above modification to your requirements file you may not even need my branch (please try to confirm). But, I would still like to preserve their order just incase this changes in the future.

And just so you know, this re-ordering/sorting logic is in place so we can detect duplicate requirements.txt files that may contain different comments, or ordering of packages but still actually mean the same thing because the order of packages in a requirements file doesn't matter.

It makes sense to skip trying to order these header/options type commands since pip requires them to be at the top, however, you would hope/assume that pip would not care about their order, eh? Honestly, this seems like a pip bug. Bug anyways, try my slight modification mentioned above on the branch I specified above and see if that fixes it for you and let me know.

Oh and sorry I couldn't get around to it yesterday! :( Ran out of time, needed to sleep

[dschep]

Thanks for the quick turn around @AndrewFarley!

[AndrewFarley]

No problem, I fully expect a few tweaks/bugs here and there since it was a rather large change. I'll do my best for quick turnaround, help keep everyone working happily.

[vickeryj]

This works well, thanks!

[paddycarey]

Hi @AndrewFarley I'm still seeing this reordering behaviour in the latest release (4.2.1) after this fix.

I have a Pipfile and when I (or the plugin) run pipenv lock --requirements --keep-outdated it spits out the requirements.txt as follows:

-i https://pypi.python.org/simple
--extra-index-url https://xxxx:@packagecloud.io/shopkeep/python/pypi/simple
attrs==18.2.0
aws-xray-sdk==2.1.0
boto3==1.9.3
botocore==1.12.3
...

but after reordering it gets changed to

--extra-index-url https://xxxxx:@packagecloud.io/shopkeep/python/pypi/simple

-i https://pypi.python.org/simple
attrs==18.2.0
aws-xray-sdk==2.1.0
cached-property==1.5.1
certifi==2018.8.24
...

which fails for the same reason as above.

Changing to -- instead of - isn't really an option because the requirements.txt is generated by Pipenv rather than stored in source control

This feels like the same issue but happy to open a new one if this isn't the right place.

[AndrewFarley]

@paddycarey Definitely the same/similar error... you can fix this right now by changing -i to --index-url or give me a few minutes and I'll have another MR for you.

Side-note, I'm going to go ahead and add -i and -f to the ignore list, found a good link to all header/options...

https://pip.pypa.io/en/stable/reference/pip_install/#requirements-file-format

[AndrewFarley]

@paddycarey Can you please try the following, in the MR linked above which hopefully @dschep can quick review/merge/release as well to help prevent this from happening to anyone else.

rm -Rf node_modules/serverless-python-requirements
npm i github:andrewfarley/serverless-python-requirements#bug-fix-requirements-options

[brett-matthews]

mean the same thing because the order of packages in a requirements file doesn't matter.

@AndrewFarley not sure that is entirely true, correct me if wrong;

https://pip.pypa.io/en/stable/reference/pip_install/#installation-order

I had a requirements file like so;

--extra-index-url https://xxxx:@packagecloud.io/fake/python/pypi/simple
private-django-package==x.x.x

django-otp==x.x.x

With private-django-package having a dependency of Django~2.2.0, this is the version of Django that will be installed.

Once reordered, django-otp having a dependency of Django>3.0, this later version of Django will now be installed.

"In the event of a dependency cycle (aka “circular dependency”), the current implementation (which might possibly change later) has it such that the first encountered member of the cycle is installed last."

Solved this by listing Django~=2.2 explicitly in my requirements.txt - (Is this what I should be doing anyway?) - but took me a little while to figure out why this was happening.

[AndrewFarley]

So maybe we need to discuss altering the logic I created that reorders the requirements file. For a little background information, that was created so that repos that used the same requirements but possibly in a different order would be considered the same and would use the same cache.

Removing this logic honestly would probably solve a few problems, maybe it was just a little too ambitious. I’d love to hear if any of the other contributors or maintainers has any objections to that. I think I’ve seen quite a bit of pain because of this. Anyone care to make a case to keep the existing logic?

[AndrewFarley]

I’ll kick out a quick PR for this and I honestly think this will solve a handful of our users problems and open issues that relate to this.

@brett-matthews thoughts?

[brett-matthews]

@AndrewFarley understand why you put reordering in there. Just thought I should share my experience for others to stumble across if they had a similar problem.

Explicitly adding Django solved it for me : )

[AndrewFarley]

@brett-matthews I suspect it only solved your problem because in your case the circular dependency you ran into happened to be solved with them in alphabetical order. If it wouldn’t have been there would be no way in which you could make it work. Which seems to be what I see some people encounter in other issues.

[brett-matthews]

ah yes you are right! this may block other people depending on how lucky you are with your dependency's name..

[AndrewFarley]

PR: #481

[brett-matthews]

thank you @AndrewFarley!