Skip to content

No longer sort generated requriements.txt file #481

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

Conversation

AndrewFarley
Copy link
Contributor

@AndrewFarley AndrewFarley commented Feb 28, 2020

Purpose

I removed the "feature" that sorts the contents of the requirements.txt file before running pip install

Reasoning

This is a long-time coming. This feature which I originally thought was quite novel was also met with a lot of problems after merging from the general public. I added this feature when I added the ability for this plugin to cache the requirements locally to prevent unnecessary re-building of requirements when the requirements.txt file did not change. At the time, I was managing roughly 5 different serverless codebases which uses the same dependencies but their requirements.txt files differed because of some dev/internal/local/nodeploy dependencies. To solve this problem I cleansed and sorted the requirements.txt file. Unfortunately, this inevitably had a LOT of issues because of edge cases I was not aware of when I first implemented it the biggest of which is that pip resolves circular dependencies by having the first package taking priority.

Result

Because of this, and this new knowledge I firmly believe we must preserve the order of the files inside the requirements.txt file, even at the risk of a bit less of a cache-hit.

Related to and/or would have prevented:

#427
#294
#238

@AndrewFarley
Copy link
Contributor Author

So idk, these tests pass locally for me... I'm bumping the CI to re-trigger it to see if that helps, if not I'll try to dig a little deeper.

@AndrewFarley
Copy link
Contributor Author

Waiting for CI to be fixed by @bsamuel-ui in #482 then will rebase

@zdk123
Copy link

zdk123 commented Jun 15, 2020

any thoughts about resurrecting this PR?

@pgrzesik
Copy link
Contributor

Hey @AndrewFarley - it's been a long time since this PR was proposed. I'm going to close it, if you feel like the issue is valid, please open a new issue or a new PR against the latest main branch. Thanks 🙇

@pgrzesik pgrzesik closed this Sep 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants