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

Support Exclude Package with custom unsafe packages #1509

Merged
merged 19 commits into from Jul 17, 2022

Conversation

hmc-cs-mdrissi
Copy link
Contributor

@hmc-cs-mdrissi hmc-cs-mdrissi commented Oct 14, 2021

#333 Closes this issue. Custom unsafe packages allows you to exclude packages. I named the option --unsafe-packages as all logic is customizing UNSAFE_PACKAGES (motivated by this comment), but if people would prefer I call it exclude that works. The one difference in the two names is whether original UNSAFE_PACKAGES remain or are gone.

As I'd ideally like setuptools/pip/etc to be in my requirements.txt file, but want to exclude other packages I prefer some way to replace the existing unsafe packages entirely. This is not a big deal to me though so if people would prefer semantic of it only adds packages to unsafe list I can do that. That is less flexible though.

I also added a flag to control whether recursive dependencies of unsafe only packages are treated as unsafe or not. The current logic for setuptools/pip is recursive dependencies only from unsafe packages are treated unsafe, but my use case for exclude is I only want to remove the exact packages I list.

Multiple occurrences of the flag are allowed. It can be used like,

pip-compile --unsafe-package foo --unsafe-package bar ...

I added two test cases to cover recursive flag being on/off.

@nvie @ssbarnea

Contributor checklist
  • Provided the tests for the changes.
  • Assure PR title is short, clear, and good to be included in the user-oriented changelog
Maintainer checklist
  • Assure one of these labels is present: backwards incompatible, feature, enhancement, deprecation, bug, dependency, docs or skip-changelog as they determine changelog listing.
  • Assign the PR to an existing or new milestone for the target version (following Semantic Versioning).

@hmc-cs-mdrissi hmc-cs-mdrissi mentioned this pull request Oct 20, 2021
@ssbarnea ssbarnea added this to the 6.5.0 milestone Oct 25, 2021
Copy link
Member

@ssbarnea ssbarnea left a comment

Choose a reason for hiding this comment

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

LGTM and I even tested it on a project where I wanted to use that feature and worked as expected.

Copy link
Member

@atugushev atugushev left a comment

Choose a reason for hiding this comment

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

Generally, it looks good. I have a concern though:

piptools/scripts/compile.py Outdated Show resolved Hide resolved
@ssbarnea ssbarnea requested a review from webknjaz January 4, 2022 14:17
@di
Copy link
Contributor

di commented Feb 4, 2022

I see this is in the 6.5.0 milestone, is this blocking a 6.5.0 release?

@ssbarnea
Copy link
Member

ssbarnea commented Feb 4, 2022

I would really want to see this feature in ASAP as lack of it seriously limited tool use in several projects.

@atugushev
Copy link
Member

It does not block the 6.5.0. Let's release it later.

@hmc-cs-mdrissi
Copy link
Contributor Author

Is thee anything that's recommended I should do for this PR? Should I deal with dependencies of dependencies logic or should I leave it as out of scope? If there's another commit I can do to help merge this that would be helpful. It'd be nice to use this without fork.

@atugushev atugushev removed this from the 6.6.0 milestone Apr 4, 2022
tests/conftest.py Outdated Show resolved Hide resolved
tests/test_cache.py Outdated Show resolved Hide resolved
@ssbarnea
Copy link
Member

ssbarnea commented Jul 7, 2022

Any chance to resolve current conflicts? I would really want to see this feature in.

@hmc-cs-mdrissi
Copy link
Contributor Author

I've resolved conflicts and I think tests should pass now with 2020 resolver too.

@ssbarnea
Copy link
Member

@atugushev Please have a look at this and if ok-ish lets get it in before it needs other rebases.

Copy link
Member

@atugushev atugushev left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM 👍🏻

hmc-cs-mdrissi and others added 2 commits July 16, 2022 20:46
@atugushev
Copy link
Member

@hmc-cs-mdrissi thanks for cleaning up reverse_dependencies(). Could you also consider this hmc-cs-mdrissi#1?

@ssbarnea
Copy link
Member

@atugushev I would merge it as is and let him fix after. Because that the first contribution even CI is not running until one of us is approving it.

@atugushev
Copy link
Member

I would merge it as is and let him fix after. Because that the first contribution even CI is not running until one of us is approving it.

@ssbarnea sure! Feel free to.

@ssbarnea
Copy link
Member

@atugushev That pip cache step is hilarous almost two hours and still running https://github.com/jazzband/pip-tools/runs/7379969275?check_suite_focus=true

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.

None yet

5 participants