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

Use pep517 to load project metadata #1311

Merged
merged 8 commits into from Mar 6, 2021

Conversation

astrojuanlu
Copy link
Contributor

@astrojuanlu astrojuanlu commented Jan 30, 2021

Fix #908, first step towards addressing #1047.
Fix #1139.

As per title: this uses pep517 as suggested in pypa/setuptools#1951 (comment) (and mentioned in #908 and #1047).

I did not add any new tests since I did not change any functionality, this is an internal refactor. On the other hand, this is my first contribution to pip-tools so I tried to keep the changes as minimal as possible. If maintainers think that I should also address #1047 in this PR before merging it, I can give it a try.

Changelog-friendly one-liner: Use pep517 to load project metadata

Contributor checklist
  • Provided the tests for the changes.
  • 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
Copy link

codecov bot commented Jan 30, 2021

Codecov Report

Merging #1311 (48c3e0f) into master (21d56e4) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1311   +/-   ##
=======================================
  Coverage   99.62%   99.62%           
=======================================
  Files          33       33           
  Lines        2915     2928   +13     
  Branches      318      319    +1     
=======================================
+ Hits         2904     2917   +13     
  Misses          5        5           
  Partials        6        6           
Impacted Files Coverage Δ
piptools/scripts/compile.py 100.00% <100.00%> (ø)
tests/test_cli_compile.py 100.00% <100.00%> (ø)
piptools/repositories/pypi.py 97.25% <0.00%> (-0.02%) ⬇️

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 21d56e4...3375cd2. Read the comment docs.

@astrojuanlu astrojuanlu force-pushed the use-pep517-load-metadata branch 2 times, most recently from 9bb6390 to 86b78c1 Compare January 30, 2021 12:15
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.

Thanks for the contribution! I've skimmed the PR and have a few comments below.

piptools/scripts/compile.py Outdated Show resolved Hide resolved
piptools/scripts/compile.py Outdated Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
@astrojuanlu astrojuanlu force-pushed the use-pep517-load-metadata branch 3 times, most recently from 0323f80 to a50f480 Compare February 2, 2021 15:43
@astrojuanlu
Copy link
Contributor Author

Addressed comments, thanks for your review and guidance @atugushev !

Copy link
Contributor

@orsinium orsinium left a comment

Choose a reason for hiding this comment

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

Great work! That's a small change with a huge impact. Basically, it provides lock file support for flit. I'm not a maintainer, though, so my opinion means not too much. Is there anything I can help with?

piptools/scripts/compile.py Outdated Show resolved Hide resolved
@atugushev
Copy link
Member

atugushev commented Feb 15, 2021

I think we can go ahead and merge this as is. Though, I'm a little worried because we don't have enough tests for pip-compile setup.py functionality.

@atugushev atugushev added the enhancement Improvements to functionality label Feb 15, 2021
@atugushev atugushev added the setuptools Related to compiling requirements form setup.py label Feb 26, 2021
@atugushev atugushev added this to the 6.0.0 milestone Feb 26, 2021
@astrojuanlu
Copy link
Contributor Author

@atugushev Do you want me to rebase and have a look at the CI? Or will you take care of it?

@atugushev
Copy link
Member

@astrojuanlu it's okay, no need to rebase. Could you please take a look at what went wrong?

@atugushev
Copy link
Member

@astrojuanlu there is a fix: astrojuanlu#1

@atugushev
Copy link
Member

Also added a test that pip-compile preserves environment markers, which means that this PR also resolves #1139 🎉. Updated description.

@astrojuanlu
Copy link
Contributor Author

@atugushev I see that we removed disable-pip-version-check = True in your PR and that made the tests fail. I'll bring it back if you're OK with it.

@astrojuanlu
Copy link
Contributor Author

Sigh... that made things worse, sorry for the noise. Let me see if I can fix this locally before bothering the CI.

@astrojuanlu
Copy link
Contributor Author

When running the tests locally with my previous commit, I get this output:

ERROR:   py36-pip20.1-coverage: commands failed
ERROR:   py36-pip20.2-coverage: commands failed
ERROR:   py36-pip20.3-coverage: commands failed
ERROR:   py36-pipprevious-coverage: commands failed
  py36-piplatest-coverage: commands succeeded
ERROR:   py36-pipmaster-coverage: commands failed
ERROR:   py37-pip20.1-coverage: commands failed
ERROR:   py37-pip20.2-coverage: commands failed
ERROR:   py37-pip20.3-coverage: commands failed
ERROR:   py37-pipprevious-coverage: commands failed
  py37-piplatest-coverage: commands succeeded
ERROR:   py37-pipmaster-coverage: commands failed
ERROR:   py38-pip20.1-coverage: commands failed
ERROR:   py38-pip20.2-coverage: commands failed
ERROR:   py38-pip20.3-coverage: commands failed
ERROR:   py38-pipprevious-coverage: commands failed
  py38-piplatest-coverage: commands succeeded
ERROR:   py38-pipmaster-coverage: commands failed
ERROR:   py39-pip20.1-coverage: commands failed
ERROR:   py39-pip20.2-coverage: commands failed
ERROR:   py39-pip20.3-coverage: commands failed
ERROR:   py39-pipprevious-coverage: commands failed
  py39-piplatest-coverage: commands succeeded
ERROR:   py39-pipmaster-coverage: commands failed
SKIPPED:  pypy3-pip20.1-coverage: InterpreterNotFound: pypy3
SKIPPED:  pypy3-pip20.2-coverage: InterpreterNotFound: pypy3
SKIPPED:  pypy3-pip20.3-coverage: InterpreterNotFound: pypy3
SKIPPED:  pypy3-pipprevious-coverage: InterpreterNotFound: pypy3
SKIPPED:  pypy3-piplatest-coverage: InterpreterNotFound: pypy3
SKIPPED:  pypy3-pipmaster-coverage: InterpreterNotFound: pypy3
  checkqa: commands succeeded
  readme: commands succeeded

Therefore, tests are only passing on the latest pip version. Now that dependencies get installed with pip through pep517, I think we need to globally disable the pip version check. But I wonder if this is an acceptable thing to do, or to what level of granularity should we control it.

@atugushev
Copy link
Member

@astrojuanlu I would disable this check globally since it looks not useful for pip-tools. Possible solutions:

  • set environment in pytest_configure
  • add a fixture with monkeypatch.setenv which runner would use
  • [tool:pytest] filterwarnings in setup.cfg

This was referenced Mar 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvements to functionality setuptools Related to compiling requirements form setup.py
Projects
None yet
5 participants