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

Complete pip-tools setup #5486

Merged
merged 1 commit into from Feb 28, 2021
Merged

Complete pip-tools setup #5486

merged 1 commit into from Feb 28, 2021

Conversation

greshilov
Copy link
Contributor

What do these changes do?

Solve problems with inconsistent approaches to installing dependencies in Makefile.

Are there changes in behavior for the user?

No.

Related issue number

Related PR: #5470.

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt
    • The format is <Name> <Surname>.
    • Please keep alphabetical order, the file is sorted by names.
  • Add a new news fragment into the CHANGES folder
    • name it <issue_id>.<type> for example (588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the pr
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Feb 20, 2021
@greshilov
Copy link
Contributor Author

@webknjaz I've marked this PR with WIP because I'm not sure about pip-sync usage. It seemed logical to use it since we moved to pip-compile, what do you think?

@greshilov greshilov force-pushed the fix-makefile branch 2 times, most recently from 1b976e3 to 4ebd012 Compare February 20, 2021 20:14
@codecov
Copy link

codecov bot commented Feb 20, 2021

Codecov Report

Merging #5486 (7cbe6fd) into master (8c82ba1) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #5486   +/-   ##
=======================================
  Coverage   97.17%   97.17%           
=======================================
  Files          41       41           
  Lines        8857     8857           
  Branches     1424     1424           
=======================================
  Hits         8607     8607           
  Misses        133      133           
  Partials      117      117           
Flag Coverage Δ
unit 97.06% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


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 8c82ba1...7cbe6fd. Read the comment docs.

@greshilov
Copy link
Contributor Author

Looks like some tests are failing because of new warnings introduced in 77d0366.

Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

I don't use pip-sync and it's a good idea to make sure that this all works with vanilla pip.

Also, for dependabot to work properly, we need to turn all the direct dependency files into .in. The pip-tools setup is incomplete and it needs polishing first before we even consider integrating extra things.
Take a look at pypa/warehouse — they have a good example of the file pairs for dependabot.

requirements/cython.txt Outdated Show resolved Hide resolved
requirements/cython.txt Show resolved Hide resolved
@greshilov
Copy link
Contributor Author

greshilov commented Feb 21, 2021

Summarizing the changes:

  1. Files doc-spelling,lint,cython,dev are split into in,txt pairs. All of them are used as a direct requirement somewhere (lint in workflow script e.t.c.).
    Note the order, lint file must be always compiled before dev.

  2. Unfortunately we can't use -r dev.in -c dev.txt anymore, because of the new pip resolver implementation. That's not a problem, I guess. pip-compile is designed to produce ready-to-go .txt dependencies.

  3. I added the .update-pip step before all installation steps.

@greshilov greshilov changed the title [WIP] Use pip-sync tool by default. [WIP] Complete pip-tools setup Feb 22, 2021
@webknjaz
Copy link
Member

  • Unfortunately we can't use -r dev.in -c dev.txt anymore, because of the new pip resolver implementation. That's not a problem, I guess. pip-compile is designed to produce ready-to-go .txt dependencies.

Why is that? What exactly causes problems?

@greshilov
Copy link
Contributor Author

greshilov commented Feb 28, 2021

  • Unfortunately we can't use -r dev.in -c dev.txt anymore, because of the new pip resolver implementation. That's not a problem, I guess. pip-compile is designed to produce ready-to-go .txt dependencies.

Why is that? What exactly causes problems?

We have a requirement pyjwt[crypto]==2.0.0, which is declared with the extra dependency [crypto]. pip-compile carefully includes this requirement in dev.txt, but new pip resolver rules strictly prohibit any extras in a constraint file.

Generally speaking any dependency with extra will break the approach with -r dev.in -c dev.txt :(
Read more about the reasons behind this behavior here.

@webknjaz
Copy link
Member

@greshilov sounds like pip-tools should be fixed to generate a proper constraints file...

@webknjaz
Copy link
Member

Meanwhile, we'll have to use -r req.txt syntax and probably generate separate requirements for different envs

@greshilov
Copy link
Contributor Author

Meanwhile, we'll have to use -r req.txt syntax and probably generate separate requirements for different envs

This PR is alright for now then? I also wanted to suggest adding hashes to lock files, like in pypa/warehouse and mozilla/ichnaea.

@greshilov greshilov changed the title [WIP] Complete pip-tools setup Complete pip-tools setup Feb 28, 2021
@webknjaz
Copy link
Member

This PR is alright for now then?

Alright. Let's merge it then, but we still need to raise this issue in the pip-tools repo, I think.
Also, I suppose you'll backport this, right?

I also wanted to suggest adding hashes to lock files, like in pypa/warehouse and mozilla/ichnaea.

That's a great idea! I do this in all of my projects by default.

@webknjaz webknjaz merged commit 23ef73e into aio-libs:master Feb 28, 2021
@@ -0,0 +1 @@
Complete pip-tools setup.
Copy link
Member

@webknjaz webknjaz Feb 28, 2021

Choose a reason for hiding this comment

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

Urgh... This is not a bugfix but a misc change because it's not a user-facing fix but something that would only affect the core devs and contributors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, my bad!

Copy link
Member

Choose a reason for hiding this comment

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

It's not a big deal, I guess this can be fixed by a follow-up PR with git mv.

@greshilov greshilov mentioned this pull request Feb 28, 2021
5 tasks
greshilov added a commit to greshilov/aiohttp that referenced this pull request Feb 28, 2021
greshilov added a commit to greshilov/aiohttp that referenced this pull request Mar 1, 2021
@webknjaz webknjaz mentioned this pull request Mar 1, 2021
5 tasks
greshilov added a commit to greshilov/aiohttp that referenced this pull request Mar 2, 2021
greshilov added a commit to greshilov/aiohttp that referenced this pull request Mar 2, 2021
greshilov added a commit to greshilov/aiohttp that referenced this pull request Mar 2, 2021
greshilov added a commit to greshilov/aiohttp that referenced this pull request Mar 2, 2021
greshilov added a commit to greshilov/aiohttp that referenced this pull request Mar 2, 2021
greshilov added a commit to greshilov/aiohttp that referenced this pull request Mar 2, 2021
greshilov added a commit to greshilov/aiohttp that referenced this pull request Mar 2, 2021
greshilov added a commit to greshilov/aiohttp that referenced this pull request Mar 2, 2021
greshilov added a commit to greshilov/aiohttp that referenced this pull request Mar 2, 2021
greshilov added a commit to greshilov/aiohttp that referenced this pull request Mar 2, 2021
greshilov added a commit to greshilov/aiohttp that referenced this pull request Mar 3, 2021
greshilov added a commit to greshilov/aiohttp that referenced this pull request Mar 3, 2021
greshilov added a commit to greshilov/aiohttp that referenced this pull request Mar 3, 2021
webknjaz pushed a commit that referenced this pull request Mar 14, 2021
commonism pushed a commit to commonism/aiohttp that referenced this pull request Apr 27, 2021
commonism pushed a commit to commonism/aiohttp that referenced this pull request Apr 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided There is a change note present in this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants