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

Fix allow unsafe false pinning bug v2 #517

Merged

Conversation

dschaller
Copy link
Member

@dschaller dschaller commented May 28, 2017

Fix a bug introduced in #441 that pinned unsafe dependencies in generated requirements files when --allow-unsafe was False.

I will add the changes to the CHANGELOG if we agree this is the correct approach.

Contributor checklist
  • Provided the tests for the changes
  • Added the changes to CHANGELOG.md
  • Requested (or received) a review from another contributor

@dschaller dschaller force-pushed the fix-allow-unsafe-false-pinning-bug-V2 branch from 67821b8 to 69ac861 Compare May 28, 2017 02:02
@dschaller dschaller mentioned this pull request May 28, 2017
3 tasks
@dschaller dschaller force-pushed the fix-allow-unsafe-false-pinning-bug-V2 branch from 69ac861 to 549edc0 Compare May 28, 2017 02:07
@dschaller
Copy link
Member Author

dschaller commented May 28, 2017

Ran a few things to test these changes:
Setup:

mktmpenv
pip install pip-tools
mkdir pkg && cd pkg
cat <<EOF > setup.py
from setuptools import setup
setup(install_requires=['setuptools'])
EOF
cd ..
echo "-e pkg" | pip-compile -n -o r.txt -

Run:

 > echo "-e pkg" | pip-compile -n -o r.txt - --allow-unsafe
#
# This file is autogenerated by pip-compile
# To update, run:
#
#    pip-compile --output-file r.txt -
#
-e file:///Users/dschaller/Desktop/testing/pkg
appdirs==1.4.3            # via setuptools
packaging==16.8           # via setuptools
pyparsing==2.2.0          # via packaging
six==1.10.0               # via packaging, setuptools

# The following packages are considered to be unsafe in a requirements file:
setuptools==35.0.2
 > echo "-e pkg" | pip-compile -n -o r.txt -
#
# This file is autogenerated by pip-compile
# To update, run:
#
#    pip-compile --output-file r.txt -
#
-e file:///Users/dschaller/Desktop/testing/pkg

Setup:

requests==2.0.0
setuptools==35.0.2
pip==8.0.2

Run:

> pip-compile --allow-unsafe
#
# This file is autogenerated by pip-compile
# To update, run:
#
#    pip-compile --output-file requirements.txt requirements.in
#
appdirs==1.4.3            # via setuptools
packaging==16.8           # via setuptools
pyparsing==2.2.0          # via packaging
requests==2.0.0
six==1.10.0               # via packaging, setuptools

# The following packages are considered to be unsafe in a requirements file:
pip==8.0.2
setuptools==35.0.2
 > pip-compile
#
# This file is autogenerated by pip-compile
# To update, run:
#
#    pip-compile --output-file requirements.txt requirements.in
#
requests==2.0.0

# The following packages are considered to be unsafe in a requirements file:
# pip
# setuptools

@dschaller dschaller force-pushed the fix-allow-unsafe-false-pinning-bug-V2 branch from 2d6b4ac to 066f0ea Compare May 28, 2017 02:49
@dschaller
Copy link
Member Author

@davidovich @vphilippon @jdufresne please take a look at this approach. It fixes the bug and maintains existing functionality.

@davidovich
Copy link
Contributor

@dschaller I really appreciate the style of this new attempt, but I am still very not convinced that pip-tools should go down this route (even if a past PR made it in, that does not mean that was a wanted direction).

Aside from a better implementation (IMHO), the main question resides from the last PR, why do you absolutely need to put a known unsafe in your requirements.in and not want it in the compiled requirements? Why just not write it in the first place ?

@dschaller
Copy link
Member Author

@davidovich there were several comments on the last pull request that outlined why this is a valid use case. The entire point of the flag is to pin these dependencies as noted in the help output. It does not say that this is only for transitive dependencies.

Also, if this functionality is no longer supported you should have released a major version change following semantic guidelines since this is a backward incompatible change in behavior.

@ryan-lane
Copy link

@davidovich as far as I can tell the behavior that @dschaller is putting back in was the original intended behavior. A change came in that broke the intended behavior and changed it to match what you'd prefer. A change in behavior, even if it is better in your view, is not great as it's a breaking change for every user who was relying on the old behavior.

We should make sure the current behavior matches the old behavior, then we should look into how we can make everyone happy. Right now there's a regression and the right thing to do is fix the regression.

@jdufresne
Copy link
Member

jdufresne commented May 30, 2017

there were several comments on the last pull request that outlined why this is a valid use case. The entire point of the flag is to pin these dependencies as noted in the help output. It does not say that this is only for transitive dependencies.

IMO, the use cases were stated but never adequately explained. They mostly boiled down to "we do this" without explaining the why. I think if someone that uses this feature explains why this is critical to their development workflow, it would be easier to understand how best to review these PRs in order to help everyone.

@dschaller
Copy link
Member Author

Having unsafe dependencies in your requirements source allows for users to create generated requirements files that can be used in different environments where pining these packages is either wanted or not.
e.g.

> echo requirements.in
requests==2.0.0
setuptools==35.0.2
pip==8.0.2
 > pip-compile requirements.in -o a-requirements.txt
#
# This file is autogenerated by pip-compile
# To update, run:
#
#    pip-compile --output-file a-requirements.txt requirements.in
#
requests==2.0.0

# The following packages are considered to be unsafe in a requirements file:
# pip
# setuptools
> pip-compile requirements.in -o b-requirements.txt --allow-unsafe
#
# This file is autogenerated by pip-compile
# To update, run:
#
#    pip-compile --output-file b-requirements.txt requirements.in
#
appdirs==1.4.3            # via setuptools
packaging==16.8           # via setuptools
pyparsing==2.2.0          # via packaging
requests==2.0.0
six==1.10.0               # via packaging, setuptools

# The following packages are considered to be unsafe in a requirements file:
pip==8.0.2
setuptools==35.0.2
Environment A:
> pip install a-requirements.txt
Environment B:
> pip install b-requirements.txt

This allows for a single source of truth on what requirements are needed for a project while also not forcing people to consume what are considered unsafe packages when it doesn't matter if these are pinned.

If you look at the comments on the original PR there are at least two other people who had use-cases that required this flag as well.

Copy link
Member

@vphilippon vphilippon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

This was an unexpected change, breaks the previous behaviour, and we have a report of it as a bug by a user, who sumbmited a PR to fix it. This should go through IMO.

We can discuss what changes we want in a seperate PR, and bring a 2.0 if we want to do so (after releasing at least one version with this fix, of course).

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.

5 participants