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

bazel: Make use of new incremental pip installer #18026

Merged
merged 2 commits into from
Sep 10, 2021

Conversation

phlax
Copy link
Member

@phlax phlax commented Sep 8, 2021

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

Commit Message: bazel: Make use of new incremental pip installer
Additional Description:

this rationalizes the pip dependencies down to ~1 requirements file (there are a few others left for non bazel requirements atm)

also removes dep comments, as previously requested by @moderation

Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Sep 8, 2021
@repokitteh-read-only
Copy link

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).

🐱

Caused by: #18026 was opened by phlax.

see: more, trace.

@phlax phlax requested a review from moderation September 8, 2021 12:21
@phlax phlax force-pushed the bazel-pip-incremental branch from e29183f to eff3c18 Compare September 8, 2021 13:27
@phlax phlax force-pushed the bazel-pip-incremental branch from eff3c18 to 85bcf4a Compare September 8, 2021 13:31
@phlax phlax force-pushed the bazel-pip-incremental branch 4 times, most recently from 892a26f to c3f37a8 Compare September 8, 2021 14:19
@phlax
Copy link
Member Author

phlax commented Sep 8, 2021

8( this seems to be blocked by bazelbuild/rules_python#527

@zuercher
Copy link
Member

zuercher commented Sep 8, 2021

/wait

@phlax phlax force-pushed the bazel-pip-incremental branch 2 times, most recently from 2cebac2 to 9b936dc Compare September 9, 2021 10:24
@phlax
Copy link
Member Author

phlax commented Sep 9, 2021

unblocked by using upstream main/HEAD

@phlax phlax force-pushed the bazel-pip-incremental branch 10 times, most recently from 67d1b77 to c20579f Compare September 9, 2021 14:36
ci/do_ci.sh Outdated Show resolved Hide resolved
ci/do_ci.sh Outdated Show resolved Hide resolved
ci/do_ci.sh Outdated Show resolved Hide resolved
Signed-off-by: Ryan Northey <ryan@synca.io>
@phlax phlax force-pushed the bazel-pip-incremental branch from c20579f to 5ee6d61 Compare September 9, 2021 14:52
@zuercher
Copy link
Member

zuercher commented Sep 9, 2021

/assign-from @envoyproxy/dependency-shepherds

@repokitteh-read-only
Copy link

@envoyproxy/dependency-shepherds assignee is @mattklein123

🐱

Caused by: a #18026 (comment) was created by @zuercher.

see: more, trace.

@phlax
Copy link
Member Author

phlax commented Sep 9, 2021

following up offline conversation with @htuch - this currently removes the requirements.txt from the kafka extension

we may, for purposes of code ownership, want to keep some requirements.txt files separate - like, eg, the kafka one

mho is that we (want to) maintain the deps across the repo, and that doesnt tend to be so related to codeownership, more dependency shepherding, so for the sake of making the latter easier, the less dependabotted files the better

mattklein123
mattklein123 previously approved these changes Sep 9, 2021
@moderation
Copy link
Contributor

/lgtm deps

@repokitteh-read-only repokitteh-read-only bot removed the deps Approval required for changes to Envoy's external dependencies label Sep 10, 2021
htuch
htuch previously approved these changes Sep 10, 2021
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

LGTM, but I do remain a little uneasy at the lack of versioning in requirements.in. I'm going to go with what you said in your other review and accept that we probably don't care most of the time and when we do, we have the option to pin.

Signed-off-by: Ryan Northey <ryan@synca.io>
@phlax phlax dismissed stale reviews from htuch and mattklein123 via d3318dd September 10, 2021 09:36
@phlax
Copy link
Member Author

phlax commented Sep 10, 2021

@mattklein123 i had to readd the requirements file for code_format

@phlax
Copy link
Member Author

phlax commented Sep 10, 2021

I'm going to go with what you said in your other review and accept that we probably don't care most of the time and when we do, we have the option to pin.

yep, we can

not sure if you saw my other comment about cross-platform compilation of the lock file

atm pip-compile doesnt do the right thing on this, so if you want eg a package only on windows, you need to add it without any predicates, and then add in bazel so its only loaded in win

@phlax phlax merged commit ede719d into envoyproxy:main Sep 10, 2021
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