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

Add compatibility with the pip master (upcoming pip==19.3) #864

Merged
merged 6 commits into from
Sep 19, 2019

Conversation

atugushev
Copy link
Member

@atugushev atugushev commented Aug 2, 2019

Changelog-friendly one-liner: Add compatibility with pip==19.3

Contributor checklist
  • Provided the tests for the changes.
  • Requested a review from another contributor.
  • Gave a clear one-line description in the PR (that the maintainers can add to CHANGELOG.md on release).
  • Assign the PR to an existing or new milestone for the target version (following Semantic Versioning).

@codecov

This comment has been minimized.

@codecov
Copy link

codecov bot commented Aug 2, 2019

Codecov Report

Merging #864 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #864      +/-   ##
==========================================
+ Coverage   99.05%   99.06%   +<.01%     
==========================================
  Files          35       35              
  Lines        2230     2237       +7     
  Branches      285      286       +1     
==========================================
+ Hits         2209     2216       +7     
  Misses         13       13              
  Partials        8        8
Impacted Files Coverage Δ
piptools/utils.py 100% <100%> (ø) ⬆️
piptools/repositories/pypi.py 94.84% <100%> (+0.02%) ⬆️
tests/test_repositories.py 100% <100%> (ø) ⬆️
tests/test_cli_compile.py 100% <100%> (ø) ⬆️
piptools/scripts/compile.py 100% <100%> (ø) ⬆️
tests/test_utils.py 100% <100%> (ø) ⬆️
piptools/resolver.py 100% <0%> (ø) ⬆️
tests/conftest.py 97.7% <0%> (ø) ⬆️
tests/test_resolver.py 100% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d6d631e...8b05751. Read the comment docs.

Copy link
Contributor

@blueyed blueyed left a comment

Choose a reason for hiding this comment

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

To be in line with the improvements in pip (lazy import / creation), I think it would be good if _compat.pip_compat would do less also - specifically not doing the InstallCommand = do_import("commands.install", "InstallCommand") anymore. This should be done via/in create_install_command then only.

piptools/utils.py Outdated Show resolved Hide resolved
@atugushev atugushev added this to the 4.0.1 milestone Aug 5, 2019
@atugushev atugushev modified the milestones: 4.0.1, 4.1.0 Aug 14, 2019
@atugushev
Copy link
Member Author

atugushev commented Aug 17, 2019

Master broken again, see #876

@atugushev atugushev changed the title Add compatibility with upcoming pip==19.3 Add compatibility with the pip master (upcoming pip==19.3) Aug 17, 2019
@atugushev
Copy link
Member Author

atugushev commented Aug 17, 2019

@blueyed

I've used do_import and added compatibility for the latest master changes. Please take a look if you have time.

@atugushev
Copy link
Member Author

The PackageFinder has been refactored pypa/pip#6787

@atugushev atugushev removed this from the 4.1.0 milestone Aug 25, 2019
@atugushev atugushev closed this Sep 6, 2019
@atugushev atugushev reopened this Sep 6, 2019
@atugushev
Copy link
Member Author

atugushev commented Sep 6, 2019

FTR, Resolver is going to be refactored pypa/pip#6986 and might break instantination

resolver_kwargs = {
"finder": self.finder,
"session": self.session,
"upgrade_strategy": "to-satisfy-only",
"force_reinstall": False,
"ignore_dependencies": False,
"ignore_requires_python": False,
"ignore_installed": True,
"isolated": False,
"wheel_cache": wheel_cache,
"use_user_site": False,
}

@atugushev
Copy link
Member Author

atugushev commented Sep 15, 2019

I've fixed the latest compatibility issue. I hope it's the last breaking change in upcoming pip==19.3.

We should check this before release in October, see pypa/pip#6993 (comment). Otherwise, it'll break pip-tools again.

/cc @vphilippon @davidovich @blueyed @jdufresne @jcushman @codingjoe

Sorry for disturbing you guys. Pinging the most recently active members 🙏

Copy link
Member

@codingjoe codingjoe left a comment

Choose a reason for hiding this comment

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

LGTM, I would probably prefer it though, if you would skip the broken pytest version in a separate PR and rebase. Is there maybe a way to already test against the new version in the tox setup? I mean it does test against master, but we should maybe also add the version you wrote the exception handling for later.

@atugushev
Copy link
Member Author

Hey @codingjoe, thanks for reviewing this!

LGTM, I would probably prefer it though, if you would skip the broken pytest version in a separate PR and rebase.

Do I understand correctly that you want to remove 9f02db5 from this PR and open a separate PR for this commit?

Is there maybe a way to already test against the new version in the tox setup? I mean it does test against master, but we should maybe also add the version you wrote the exception handling for later.

Do you mean to add to tox.ini new env pip19.3 pointing to the master? For example:

pip19.3: -e git+https://github.com/pypa/pip.git@master#egg=pip

And change it later to pip19.3: pip==19.3 after the release. Right?

@codingjoe
Copy link
Member

@atugushev yes, yes and yes ;)

Function is_vcs_url has been replaced by is_vcs Link property.

See pypa/pip#6883
The `get_best_candidate` has been renamed to `compute_best_candidate`,
which now returns an instance of `BestCandidateResult`.

See pypa/pip#6787
Add pip19.3 the tox envlist.
@atugushev
Copy link
Member Author

atugushev commented Sep 17, 2019

@codingjoe

@atugushev yes, yes and yes ;)

Addressed in 8b05751. Also, I've rebased to the latest master.

Copy link
Member

@codingjoe codingjoe left a comment

Choose a reason for hiding this comment

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

LGTM 👍 thanks for adressing my change requests

@atugushev atugushev added this to the 4.2.0 milestone Sep 19, 2019
@atugushev atugushev merged commit 53c0742 into jazzband:master Sep 19, 2019
@atugushev atugushev deleted the create-command branch September 19, 2019 09:54
@atugushev
Copy link
Member Author

@codingjoe thanks for reviewing this. Appreciate it!

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: function is_vcs_url moved to is_vcs Link property
3 participants