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

Exclude packages required only by unsafe packages #441

Merged
merged 1 commit into from
Mar 30, 2017
Merged

Exclude packages required only by unsafe packages #441

merged 1 commit into from
Mar 30, 2017

Conversation

jdufresne
Copy link
Member

@jdufresne jdufresne commented Jan 24, 2017

Recent versions of setuptools have dependencies:

https://github.com/pypa/setuptools/blob/6d89f362c9657e12fe2500c061e77747305b76e4/setup.py#L165-L168

If a package requires setuptools, it is commented out in the compiled
requirements.txt. As setuptools isn't included, its dependencies should also
not be included.

Should fix #445

suutari-ai added a commit to suutari/prequ that referenced this pull request Feb 9, 2017
Mark the merged PRs of pip-tools in Change Log more clearly so that it
is easier to keep track which fixes/features are already in.

This includes following 7 PRs:

  * jazzband#355
  * jazzband#378
  * jazzband#389
  * jazzband#417
  * jazzband#441
  * jazzband#448
  * jazzband#450
@IvanAnishchuk
Copy link
Member

@jdufresne This is an important fix for many people. Could you please fix the travis build?

@jdufresne
Copy link
Member Author

Sure, I can revisit this.

It occurred to me that this needs to happen recursively. For example, packages included by packages included by setuptools should also not be included in the requirements. Until that is fixed, this is not ready for merge. I'll take a look at that problem once resolving the merge conflict and Travis build.

@davidovich
Copy link
Contributor

@jdufresne I am really not convinced that this needs to happen at output time. I think the analysis should take place at resllolve time. What is your take on this?

@IvanAnishchuk
Copy link
Member

@davidovich Yeah, that makes sense. Somewhere in _resolve_one_round() we could avoid adding unsafe requirements and their own requirements won't be added either.

@jdufresne
Copy link
Member Author

jdufresne commented Mar 8, 2017

The updated PR is ready for review. I have:

  • Rebased to resolve merge conflicts
  • Reworked logic to run at resolve time instead of output time
  • As a result, we don't need allow_unsafe at output time which allowed some removal of dead code

Just a thought, is there a good use case for allow_unsafe? I was curious about removing it entirely, but I'm not sure if the backwards compatibility would be a problem for some.

suutari-ai added a commit to suutari/prequ that referenced this pull request Mar 10, 2017
@davidovich
Copy link
Contributor

@jdufresne could you add a test to show that appdirs, packaging, etc is not added to the compiled requirements due to being unsafe?

@jdufresne
Copy link
Member Author

@davidovich I have updated the test fixture to include appdirs and packaging as dependencies of setuptools. In test_resolver.py, these dependencies are not included in the resolved set of the the html5lib test case. I believe this demonstrates what you're asking for.

Let me if you had something else in mind. If so, could you provide a bit of guidance as to what the test might look like? I'd be happy to write additional tests, just want to make sure I'm adding what you're looking for.

@davidovich
Copy link
Contributor

davidovich commented Mar 24, 2017

Yes that is what I had in mind.

I will try to merge tomorrow, or next Monday.

@davidovich davidovich added this to the 1.9 milestone Mar 29, 2017
@davidovich davidovich closed this Mar 30, 2017
@davidovich davidovich reopened this Mar 30, 2017
… use

Recent versions of setuptools have dependencies:

https://github.com/pypa/setuptools/blob/6d89f362c9657e12fe2500c061e77747305b76e4/setup.py#L165-L168

As setuptools is considered unsafe and excluded by default, its dependencies
should also be excluded.

Fixes #445
@davidovich davidovich merged commit 6853353 into jazzband:master Mar 30, 2017
@davidovich
Copy link
Contributor

Thanks @jdufresne !

@jdufresne jdufresne deleted the setuptools-requirements branch November 24, 2017 11:17
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.

pip-compile adding appdirs, packaging etc. setuptools deps
3 participants