Skip to content

bazel: Add @envoy_repo target with VERSION AND PATH #17293

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.

Sign up for GitHub

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

Merged
lizan merged 4 commits into from
Sep 14, 2021
Merged

bazel: Add @envoy_repo target with VERSION AND PATH #17293

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2906f75
bazel: Add @envoy_repo target with VERSION AND PATH
phlax Sep 5, 2021
01d41fa
move binding to repositories.bzl
phlax Sep 7, 2021
0fa28c0
remove-path-bzl
phlax Sep 8, 2021
8a7625b
move-binding
phlax Sep 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions WORKSPACE
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
workspace(name = "envoy")

load("//bazel:repo.bzl", "envoy_repo_binding")

envoy_repo_binding()
Comment thread
phlax marked this conversation as resolved.
Outdated

load("//bazel:api_binding.bzl", "envoy_api_binding")

envoy_api_binding()
Expand Down
46 changes: 46 additions & 0 deletions bazel/repo.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
def _envoy_repo_impl(repository_ctx):
"""This provides information about the Envoy repository

You can access the current version and path to the repository in .bzl/BUILD
files as follows:

```starlark

load("@envoy_repo//:version.bzl", "VERSION")
load("@envoy_repo//:path.bzl", "PATH")

@lizan lizan Sep 7, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What do you plan to use this for? I'm afraid this is an anti-pattern because wherever you pull this in rules/targets it invalidates cache across different builds.

@phlax phlax Sep 7, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

for tooling such as code_format where it needs to run across the whole repo, or where it needs to make bazel queries which imply access to the repo (ie not genqueriable)

i get that using the path in bazel is a bit of an anti-pattern, not least because its only relevant to the local machine, but we have quite a few rules that rely on having access to the repo, and currently that has to be passed around (as an absolute url), so i think from a pattern pov this would be preferable to what we do now

im not sure if iiuc the point about invalidating caches, but i dont think running dep check or code formatting tools is going to have any negative lateral effects, and afaict using this with them everything is cached correctly

@phlax phlax Sep 7, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

as an example - here is some WIP code to run a bazel query from other tools:

https://github.com/envoyproxy/envoy/pull/17992/files#diff-7a7d58af3edfdbe7825cd20fb6a29494ae293d40fe25e2b9774a4aac8ee27dcfR22-R30

the tools updated in the linked PR dont currently use bazel - ie they are currently run as scripts

if i shift them to bazel then, like with several others, we need to add a path to be able to run the bazel queries

the tools that need a path are a pita to run as you always have to type ${PWD} in addtion to any args, and it increases their code/complexity having to handle the path arg. This would eliminate that for these cases

@phlax phlax Sep 7, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

perhaps i could add something to the comment to clarify any situations where there are some adverse effect from using the PATH other than for rbe

@lizan lizan Sep 8, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

My point is this should not be exported in a .bzl file as Starlark, for the tooling we only need the py_library?

@phlax phlax Sep 8, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

trying to understand further what you might mean here.

im wondering if you mean we dont need the ".bzl" part for the PATH, but we could keep the python part and only expose it to python targets

as im working mostly with python that would wfm

ill update with this change and then hopefully (if that was your meaning) we can get this landed today

@htuch htuch Sep 10, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Have you considered the trick we do in

envoy/tools/config_validation/validate_fragment.py

Line 71 in 68f674e

descriptor_path = r.Rlocation(
?

@phlax phlax Sep 10, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

that gives the path within the sandbox, not the repo, iiuc

@phlax phlax Sep 10, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

ah, i see, you can get the file path from the symlink that way

tbh, i think this is much more explicit - retrieving the repo metadata will be possible with the @envoy_repo target - which can provide version as well as path

see https://github.com/envoyproxy/envoy/pull/18021/files#diff-615ebb7b91831a9c7da2a3835e8b7c1ef89c9d2e72777cc229e128f6c8229214R51 and https://github.com/envoyproxy/envoy/pull/18021/files#diff-7a7d58af3edfdbe7825cd20fb6a29494ae293d40fe25e2b9774a4aac8ee27dcfR22 for an example

possibly the envoy_repo namespace can also provide data mappings as in #17998

@htuch htuch Sep 10, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I like the idea of @envoy_repo for things like version, I just wonder if we can pair it back to that and we don't need to do path this way.


```
`VERSION` can be used to derive version-specific rules and can be passed
to the rules.

As the `PATH` is local to the machine, it is generally only useful for
jobs that will run locally.

This can be useful for example, for tooling that needs to check the
repository, or to run bazel queries that cannot be run within the
constraints of a `genquery`.

"""
repo_path = repository_ctx.path(repository_ctx.attr.envoy_root).dirname
version = repository_ctx.read(repo_path.get_child("VERSION")).strip()
repository_ctx.file("version.bzl", "VERSION = '%s'" % version)
repository_ctx.file("path.bzl", "PATH = '%s'" % repo_path)
repository_ctx.file("__init__.py", "PATH = '%s'\nVERSION = '%s'" % (repo_path, version))
repository_ctx.file("WORKSPACE", "")
repository_ctx.file("BUILD", """
load("@rules_python//python:defs.bzl", "py_library")

py_library(name = "envoy_repo", srcs = ["__init__.py"], visibility = ["//visibility:public"])

""")

envoy_repo = repository_rule(
implementation = _envoy_repo_impl,
attrs = {
"envoy_root": attr.label(default = "@envoy//:BUILD"),
},
)

def envoy_repo_binding():
if "envoy_repo" not in native.existing_rules().keys():
envoy_repo(name = "envoy_repo")