Fix allow unsafe false pinning bug #508

dschaller · 2017-05-13T06:34:05Z

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

Contributor checklist

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

dschaller · 2017-05-13T07:05:19Z

cc @jdufresne

jdufresne · 2017-05-13T15:36:08Z

Apologies if I have introduced a regression.

Fixed bug where unsafe packages would get pinned in generated requirements files when --allow-unsafe was not set

Do you have additional details on this? I do not use the --allow-unsafe option and I do no have pinned unsafe packages in my output. So I'm curious how you triggered this. Do you have an example, minimal requirements.in? What is the command line you're using?

Here is the command I use:

$ ./venv/bin/pip-compile --version
pip-compile, version 1.9.0
$ pip-compile --upgrade --generate-hashes -o requirements.txt requirements.in
...
[no unsafe packages]

jonathanburns · 2017-05-13T19:46:30Z

CHANGELOG.md

+
+Bug Fixes:
+- Fixed bug where unsafe packages would get pinned in generated requirements files
+when `--allow-unsafe` was not set. ([#508](https://github.com/jazzband/pip-tools/pull/508)).


I think you mean WAS set, right?

Unsafe packages were being pinned in requirements files even when --allow-unsafe wasn't set instead of being comments

jdufresne · 2017-05-13T21:16:15Z

If I run with --allow-unsafe setuptools appears in my requirements.txt as:

# The following packages are considered to be unsafe in a requirements file:
setuptools==35.0.2 \
    --hash=sha256:19d753940f8cdbee7548da318f8b56159aead279ef811a6efc8b8a33d89c86a4 \
    --hash=sha256:1e55496ca8058db68ae12ac29a985d1ee2c2483a5901f7692fb68fa2f9a250fd \
    # via html5lib

Are you saying this behavior is incorrect? setuptools should not have a pinned version? If that is the case, what is the expected output?

davidovich · 2017-05-14T03:00:28Z

I believe the initial implementation was correct in removing the unsafe packages at runtime. This PR introduces a similar kind of logic in the Output writer. I am on @jdufresne side here, and I am not sure there is a regression. Can you write a test that demonstrates the failure?

davidovich · 2017-05-14T03:03:42Z

tests/test_writer.py

 from piptools.writer import OutputWriter


-@fixture
+class Writer(object):


I don't understand this refactoring of correct code.

For what it's worth, pytest now doesn't discriminate between a yield and an ordinary fixture. @fixture is only needed.

By yielding a class that I can invoke later I am allowing for keyword arguments to be passed in so that I can reuse this fixture while setting allow_unsafe.

I'll update the fixture annotation

dschaller · 2017-05-14T06:38:12Z

@jdufresne no worries! I wanted to tag you in case I was missing a use case that isn't tested.

The command I'm running is pip-compile --allow-unsafe --no-index -o piptools_requirements.txt piptools_requirements.in which correctly pins pip.

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

But if I run pip-compile --no-index -o requirements.txt requirements.in pip also gets pinned when it shouldn't.

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

@davidovich not trying to imply there are "sides" here. Just wanted to fix a bug ;) happy to fork and fix in that.

jdufresne · 2017-05-14T14:03:43Z

But if I run pip-compile --no-index -o requirements.txt requirements.in pip also gets pinned when it shouldn't.

Thanks for following up. Can you provide the requirements.in file that triggers this? Or a minimal version of it?

dschaller · 2017-05-16T07:01:52Z

@jdufresne

pip==9.0.1

davidovich · 2017-05-16T13:05:02Z

piptools/writer.py

@@ -124,9 +132,12 @@ def write(self, results, reverse_dependencies, primary_packages, markers, hashes
                    f.write(unstyle(line).encode('utf-8'))
                    f.write(os.linesep.encode('utf-8'))

-    def _format_requirement(self, ireq, reverse_dependencies, primary_packages, marker=None, hashes=None):
-        line = format_requirement(ireq, marker=marker)
+    def _format_requirement(self, ireq, reverse_dependencies, primary_packages, **kwargs):


Keep the named arguments here.

Can you provide a reason why?

Because it makes the contract clear and not hidden in the function body.

For the sake of argument it makes it harder for discoverability but it makes usage much clearer because you aren't allowing for usages to rely on position.

In terms of discoverability of arguments there should be documentation on the function with these values.

jdufresne · 2017-05-16T18:52:30Z

pip==9.0.1

I'm having trouble understanding your use case. Can you expand on it?

Why do you put pip in your requirements.in if you don't want it in your requirements.txt?

What is the point to putting pip in a requirements file anyway. As you use pip to install requirements, it won't be updated until after it is invoked.

dschaller · 2017-05-16T19:00:16Z

@jdufresne I do a first pass with pip pinned and then a second pass including the generated file from the first pass.

Even if this seems odd the functionality is broken. If someone has a unsafe library in their requirements and --allow-unsafe is not set it should not be pinned in the generated requirements file.

ryan-lane · 2017-05-16T19:53:07Z

@jdufresne I think the idea is that if an end-user puts pip into requirements.in, that we strip it unless --allow-unsafe is set. People sometimes put unsafe things into their .in file and we're ensuring that by default we try to do the right thing. The --allow-unsafe is there to let the end-user say "I know what I'm doing".

Basically, this PR just makes piptools work as advertised, based on the arguments.

davidovich · 2017-05-17T00:50:00Z

The spirit of --allow-unsafe is to control if unsafe sub-dependencies are output to the requirements.txt, not to filter a user input which is the user desired state of wanted dependencies. requirements.in state what the user wants. The --allow-unsafe flag is to remove unwanted side effects of sub-dependencies which are unsafe.

Even unsafe here is a bit hard to define.

But if the said unsafe requirements are in the input which YOU control in the requirements.in, pip-tools cannot decide to remove them for you.

I used this setup to convince myself there was no bug here. The following creates a fake package with only setuptools as sub-dependency.

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 -

The result:

#
# This file is autogenerated by pip-compile
# To update, run:
#
#    pip-compile --output-file r.t -
#
-e file:///Users/david/.virtualenvs/tmp-ef044366e6edf64/pkg
Dry-run, so nothing updated.

With --allow-unsafe:

david$ echo "-e pkg" | pip-compile --allow-unsafe -n -o r.t -
#
# This file is autogenerated by pip-compile
# To update, run:
#
#    pip-compile --output-file r.t -
#
-e file:///Users/david/.virtualenvs/tmp-ef044366e6edf64/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

dschaller · 2017-05-17T01:04:36Z

The spirit of --allow-unsafe is to control if unsafe sub-dependencies are output to the requirements.txt, not to filter a user input which is the user desired state of wanted dependencies.

@davidovich this is wrong. If you look at the PR to add the flag (#377) the description says,

Due to some specifics I had to include some version of pip in dependencies in several projects.
I am not sure it is needed for everyone, so I added a flag to have an option to enable that.

This is exactly what we are doing and what this pull request fixes...

davidovich · 2017-05-16T13:24:05Z

piptools/writer.py

+                    marker=markers.get(ireq.req.name),
+                    hashes=hashes if self.allow_unsafe else None
+                )
+                if not self.allow_unsafe:


reverse the if logic here, use the positive first.

I think it's safer to fall back to the unmodified line at the catch all.

davidovich · 2017-05-16T13:26:22Z

tests/test_writer.py

+        w, '_format_requirement', return_value='{}'.format(unsafe_package)
+    )
+    comment = mocker.patch(
+        'piptools.writer.comment', side_effect=['foobar', '# {}'.format(unsafe_package)])


Why so much mocking ? I think it is best to use the real implementation as much as possible in order to test the implementation and not the mocks.

Unit tests should test functionality in isolation of other modules assuming that those modules are tested independently.

davidovich · 2017-05-16T13:35:21Z

tests/test_writer.py

+        'piptools.writer.comment', side_effect=['foobar', '# {}'.format(unsafe_package)])
+
+    expected_results = ['header', 'flags', '', 'foobar', '# {}'.format(unsafe_package)]
+    result = w._iter_lines([ireq], False, [], {unsafe_package: 'marker'}, 'hashes')


Is the 'marker' here a real package? deduced from the USAFE_PACKAGES dependencies? or is it a test marker, something that you make up for your test ?

'marker' is an arbitrary value used for testing purposes.

davidovich · 2017-05-16T15:30:18Z

piptools/writer.py

@@ -124,9 +132,12 @@ def write(self, results, reverse_dependencies, primary_packages, markers, hashes
                    f.write(unstyle(line).encode('utf-8'))
                    f.write(os.linesep.encode('utf-8'))

-    def _format_requirement(self, ireq, reverse_dependencies, primary_packages, marker=None, hashes=None):
-        line = format_requirement(ireq, marker=marker)
+    def _format_requirement(self, ireq, reverse_dependencies, primary_packages, **kwargs):


Because it makes the contract clear and not hidden in the function body.

dschaller · 2017-05-18T00:01:49Z

Given that all the criteria in the description are satisfied, none of the feedback is critical, and most importantly this fixes a bug with intended functionality I propose that we merge this.

davidovich · 2017-05-18T00:47:25Z

@dschaller You are the only one saying the feedback is not critical. I feel the overall gist of this is that you want the change in, without regards to style or existing code.

I have made comments and tried to understand the nature of what this PR trying to fix something that I am still not convinced is broken, but they seem to be ignored.

The response I got from the example code was that I didn't read a previous PR. I understand what that PR brought to the table, but I believe we are now in a better position than at that time.

The problem is that if you put something in the input, it is not for pip-tools to decide to remove that wanted state. If you want to remove pip from the output, don't put it in the input as a wanted requirement in the first place. I sill fail to understand your use case.

I can perfectly see that I would like to put pip in the requirements.txt AND not wanting to have unsafe dependencies end up in the compiled requirements. This PR would break that.

For example, with the same setup as above, imaging I would like to have pip, so with this PR, I would need to add --allow-unsafe, but at the same time, I would not want appdirs, packaging, etc:

david$ echo -e "-e pkg\npip" | pip-compile --allow-unsafe -n -o r.t -
#
# This file is autogenerated by pip-compile
# To update, run:
#
#    pip-compile --output-file r.t -
#
-e file:///Users/david/.virtualenvs/tmp-ef044366e6edf64/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:
pip==9.0.1
setuptools==35.0.2

dschaller · 2017-05-18T03:35:51Z

@davidovich happy to chat more about the changes. My apologies if that is the way my comment came off as it was not my intent. By critical I meant the inline comments left by you were mostly style ones and they seem like a personal preference.

Please help me understand how I can I better help you understand how this is broken.

The problem is that if you put something in the input, it is not for pip-tools to decide to remove that wanted state.

Pip-tools, up until the last release with the change in it, would remove unsafe dependencies from the generated requirements files even if they were part of the original requirements unless the --allow-unsafe flag was set. I guess a better question here is was this functionality unintended then?

As far as your example goes it seems counter-intuitive that you would want pip-tools, a tool whose entire purpose is to pin all underlying dependencies, to not pin the underlying dependencies of unsafe packages when they are allowed. Can you help me understand why you wouldn't want this?

vphilippon · 2017-05-18T15:43:11Z

I went through the discussion on whether this is a bug or not, and I think I can summarize this:

@dschaller, you're pointing out that prior to 1.9.0, --allow-unsafe was an "all or nothing" option, but with the change of PR #441, we brought an unexpected change where giving an unsafe pin directly in requirements.in now stays in the output, which goes against the original idea.

@davidovich, you're saying that in retrospective, maybe thats a good change, because it allows us to say "Yes, I want pip to be pinned, but not the other unsafe ones (like setuptools)".

IMHO, I can see use cases where both behaviour would be good. And I think I have an idea to keep the best of both worlds: having a way allow specific unsafe packages, in the same way we do with --upgrade and --upgrade-package.
Giving --allow-unsafe would let all unsafe go through, but if I set --allow-unsafe-package pip instead, then only pip is allowed.
We keep the previous behaviour that our users expects, while giving more control to the users.

Whether we should do that in one or many PR is debatable (oh no... what have I done ;) ), but I feel like this is the way to go.

What do you say about this?

On another subject, @dschaller, I haven't fully analysed the code change (sorry, I'll try to!), but the feeling I got is that we went back at filtering the output rather than filtering on the input. Am I right? If so, do you think its possible to change that? I think its a better way to handle the case, and I also got a feeling that the unsafe dependencies of unsafe requirements directly in the requirements.in are going to get through, which goes against the work in PR #441.

As always, thanks everyone for your time!

dschaller · 2017-05-28T02:05:15Z

@davidovich @vphilippon @jdufresne opened #517 to hopefully fix this bug while preserving the additions in #441. Please take a look when you get a chance

jdufresne · 2017-05-31T13:15:35Z

WIth 4e2f928 merged, should this now be closed?

Derek Schaller added 5 commits May 12, 2017 23:33

fix bug where unsafe dependencies were pinned in generated requirements

828316b

add tests to prevent regressions

f34e4cd

update changelog

4585ea3

install pytest-mock

bddebd5

use existing unsafe package names in writer unit tests

4dfe7f4

dschaller force-pushed the fix-allow-unsafe-false-pinning-bug branch from ad7ff32 to 4dfe7f4 Compare May 13, 2017 06:52

fix lint issues

b0b60db

dschaller force-pushed the fix-allow-unsafe-false-pinning-bug branch from 157f3b2 to b0b60db Compare May 13, 2017 07:02

dschaller requested review from nvie and davidovich May 13, 2017 07:05

Update CHANGELOG.md

1534ca7

jonathanburns reviewed May 13, 2017

View reviewed changes

davidovich reviewed May 14, 2017

View reviewed changes

use fixture instead of pytest_fixture

33f4833

davidovich reviewed May 16, 2017

View reviewed changes

davidovich reviewed May 17, 2017

View reviewed changes

Update CHANGELOG for 1.9.1

9964775

dschaller closed this May 31, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix allow unsafe false pinning bug #508

Fix allow unsafe false pinning bug #508

dschaller commented May 13, 2017 •

edited

Loading

dschaller commented May 13, 2017

jdufresne commented May 13, 2017

jonathanburns May 13, 2017

dschaller May 14, 2017

jdufresne commented May 13, 2017 •

edited

Loading

davidovich commented May 14, 2017

davidovich May 14, 2017

dschaller May 14, 2017

dschaller commented May 14, 2017 •

edited

Loading

jdufresne commented May 14, 2017

dschaller commented May 16, 2017

davidovich May 16, 2017

dschaller May 16, 2017

davidovich May 16, 2017

dschaller May 17, 2017

jdufresne commented May 16, 2017

dschaller commented May 16, 2017 •

edited

Loading

ryan-lane commented May 16, 2017

davidovich commented May 17, 2017

dschaller commented May 17, 2017 •

edited

Loading

davidovich May 16, 2017

dschaller May 17, 2017

davidovich May 16, 2017

dschaller May 17, 2017

davidovich May 16, 2017

dschaller May 17, 2017

davidovich May 16, 2017

dschaller commented May 18, 2017

davidovich commented May 18, 2017

dschaller commented May 18, 2017 •

edited

Loading

vphilippon commented May 18, 2017 •

edited

Loading

dschaller commented May 28, 2017

jdufresne commented May 31, 2017

Fix allow unsafe false pinning bug #508

Fix allow unsafe false pinning bug #508

Conversation

dschaller commented May 13, 2017 • edited Loading

Contributor checklist

dschaller commented May 13, 2017

jdufresne commented May 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdufresne commented May 13, 2017 • edited Loading

davidovich commented May 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dschaller commented May 14, 2017 • edited Loading

jdufresne commented May 14, 2017

dschaller commented May 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdufresne commented May 16, 2017

dschaller commented May 16, 2017 • edited Loading

ryan-lane commented May 16, 2017

davidovich commented May 17, 2017

dschaller commented May 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dschaller commented May 18, 2017

davidovich commented May 18, 2017

dschaller commented May 18, 2017 • edited Loading

vphilippon commented May 18, 2017 • edited Loading

dschaller commented May 28, 2017

jdufresne commented May 31, 2017

dschaller commented May 13, 2017 •

edited

Loading

jdufresne commented May 13, 2017 •

edited

Loading

dschaller commented May 14, 2017 •

edited

Loading

dschaller commented May 16, 2017 •

edited

Loading

dschaller commented May 17, 2017 •

edited

Loading

dschaller commented May 18, 2017 •

edited

Loading

vphilippon commented May 18, 2017 •

edited

Loading