Skip to content

Conversation

phlax
Copy link
Contributor

@phlax phlax commented Apr 25, 2022

Signed-off-by: Ryan Northey ryan@synca.io

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature (please, look at the "Scope of the project" section in the README.md file)
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • [C] Other... Please describe:

Updates pip-tools to latest version

This may be a breaking change for some as more recent version of pip-compile enforce - separation of namespace package names (rather than .)

OTOH it makes it usable with dependabot which uses (at least by default) a recent version

What is the current behavior?

Cant be used with dependabot without a lot of unnecessary changes

What is the new behavior?

Hopefully at least - compiled files will be ~same as produced by dependabot so will not keep trying to revert each others changes

Does this PR introduce a breaking change?

  • Yes - of sorts
  • No

requirements.txt files compiled with compile_pip_requirements may need to be recompiled and/or fail existing test expectations

Other information

Signed-off-by: Ryan Northey <ryan@synca.io>
@mattem
Copy link
Contributor

mattem commented Apr 25, 2022

This is also a breaking change as pip-tools drops support for Python 3.6 in 6.5.0 (I know it's EoL, just a note for the diff)
Also 6.4.0 requires a minimum of pip 21.2.0 should be noted too.

@phlax
Copy link
Contributor Author

phlax commented Apr 26, 2022

@mattem lmk if i need to add anything, happy to update

testing out the PR i saw a couple of other issues that may cause issues with dependabot

one is that dependabout runs in the directory of the requirements file so all of the paths are relative to the reqs file not the WORKSPACE

re that issue im wondering if we can add some flag or similar to the rule that allows it run in the directory etc

the other issue im wondering about is the pip-compile command in the requirements.txt - its helpful that it shows the correct bazel command, but i think to prevent unnecessary changes with dependabot it needs to just be pip-compile

for ref one of the reqs files i hope to manage with in this way is here https://github.com/envoyproxy/envoy/blob/334974a0081d70b6a258f27cd40a827caf49aabf/tools/base/requirements.txt#L5

im wondering for this case we can make the command customizable in some way

(not totally certain about the command path causing issue, but i am about the relative path)

@f0rmiga
Copy link
Member

f0rmiga commented May 3, 2022

@mattem Want to guide @phlax on any specific changes it may need, or could we merge it since Python 3.6 is EOL as you mentioned?

@phlax
Copy link
Contributor Author

phlax commented Jun 1, 2022

@groodt re phlax#1 (comment) i would be happy to add to this PR but as this is a ~breaking change its maybe best to pass this one through first/alone

@f0rmiga any chance of landing this?

its not a total solution to using rules_python with dependabot but its a step in the right direction (and i guess to pip-compiling more generally)

looks like this needs a main merge, which ill do now...

lmk if there is anything else that needs to be updated

@groodt
Copy link
Collaborator

groodt commented Jun 1, 2022

@phlax SGTM. Land the merge and this PR and I'll create a fresh new one.

requirement("pip"),
requirement("pip_tools"),
requirement("setuptools"),
requirement("tomli"),

Choose a reason for hiding this comment

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

How it this one used by pip-tools. I checked the install-requires of pip-tools and it isn't there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

$ echo pip-tools > requirements.in
$ pip-compile --generate-hashes --allow-unsafe requirements.in 
#
# This file is autogenerated by pip-compile with python 3.10
# To update, run:
#
#    pip-compile --allow-unsafe --generate-hashes requirements.in
#
click==8.1.3 \
    --hash=sha256:7682dc8afb30297001674575ea00d1814d808d6a36af415a82bd481d37ba7b8e \
    --hash=sha256:bb4d8133cb15a609f44e8213d9b391b0809795062913b383c62be0ee95b1db48
    # via pip-tools
pep517==0.12.0 \
    --hash=sha256:931378d93d11b298cf511dd634cf5ea4cb249a28ef84160b3247ee9afb4e8ab0 \
    --hash=sha256:dd884c326898e2c6e11f9e0b64940606a93eb10ea022a2e067959f3a110cf161
    # via pip-tools
pip-tools==6.6.2 \
    --hash=sha256:6b486548e5a139e30e4c4a225b3b7c2d46942a9f6d1a91143c21b1de4d02fd9b \
    --hash=sha256:f638503a9f77d98d9a7d72584b1508d3f82ed019b8fab24f4e5ad078c1b8c95e
    # via -r requirements.in
tomli==2.0.1 \
    --hash=sha256:939de3e7a6161af0c887ef91b7d41a53e7c5a1ca976325f429cb46ea9bc30ecc \
    --hash=sha256:de526c12914f0c550d15924c62d72abc48d6fe7364aa87328337a31007fe8a4f
    # via pep517
wheel==0.37.1 \
    --hash=sha256:4bdcd7d840138086126cd09254dc6195fb4fc6f01c050a1d7236f2630db1d22a \
    --hash=sha256:e9a504e793efbca1b8e0e9cb979a249cf4a0a7b5b8c9e8b65a5e39d49529c1c4
    # via pip-tools

# The following packages are considered to be unsafe in a requirements file:
pip==22.1.2 \
    --hash=sha256:6d55b27e10f506312894a87ccc59f280136bad9061719fac9101bdad5a6bce69 \
    --hash=sha256:a3edacb89022ef5258bf61852728bf866632a394da837ca49eb4303635835f17
    # via pip-tools
setuptools==62.3.2 \
    --hash=sha256:68e45d17c9281ba25dc0104eadd2647172b3472d9e01f911efa57965e8d51a36 \
    --hash=sha256:a43bdedf853c670e5fed28e5623403bad2f73cf02f9a2774e91def6bda8265a7
    # via pip-tools

Copy link
Collaborator

Choose a reason for hiding this comment

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

# via pep517

Choose a reason for hiding this comment

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

Ah cheers. Didn't expect pep517 to take any deps.

@phlax
Copy link
Contributor Author

phlax commented Jun 2, 2022

im guessing we want a changelog entry somewhere - as this might break some expectations

@thundergolfer
Copy link

Yes I think this would at least go in the What's Changed section of release notes.

Copy link
Collaborator

@groodt groodt left a comment

Choose a reason for hiding this comment

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

LGTM

@groodt
Copy link
Collaborator

groodt commented Jun 6, 2022

Mergeable? I’ve got a small PR to land after this one.

@thundergolfer thundergolfer merged commit 63805ab into bazel-contrib:main Jun 7, 2022
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.

5 participants