Is there no way to get this into a separate file, i.e. a build a recipe? It's easier to read than being inlined here, where it's a mix of Bazel macros and shell.

Not with new_http_archive, but using a smarter repository definer should let us put in additional files during the "fetch" step. I'll try to get that figured out.

I would caution against adding repository rules that perform build, or you might hit the issue that led to the single massive recursive make repository rule in the current implementation. Repository rules execute sequentially, so you don't get any cross dependency build parallelism. This slowed down the initial fetch significantly, e.g. from a couple of minutes to tens of minutes.

I was thinking of using ctx.symlink() to make a shell script available within the repository, similar to how the Skylark new_http_archive works. I definitely want to avoid doing any build actions within the repository rule implementation.

Well, that's what we do today https://github.com/envoyproxy/envoy/blob/master/bazel/repositories.bzl#L12. I'm starting to wonder how the new approach (if we're moving to repository rules for everything) is moving away from the existing approach, other than decomposing the monolith recursive make, which provide parallelism via job control, to a bunch of repository rules that will be executed sequentially with no job control.

The future repository rules should be very fast (just unpacking a tarball, maybe writing a shell blob). I want all the real build work to happen within standard BUILD rules, so they can be parallelized.

That could work. I'm still a little unsure on how this will work though with output file determination, since output files may only be known following the build process (even headers might be generated), so figuring out the output artifacts is going to require a post-build step.

This is one of the reasons we opted for using a repository_rule instead of genrule. You have to manually enumerate and maintain the complete list of outputs in genrules. For simple cases it's OK, but when you have a large set of headers, which is the case in some libs, it becomes really big.

ACK. I think it should be possible to avoid huge manual enumerations by using glob(), since it's uncommon for large numbers of output files to use arbitrary names. For example, a library that installed ~dozens of proto headers could use glob("**/*.proto") to derive the output names.

I don't think genrules support globs on outputs FYI, at least last time I checked. Maybe this has changed?

See https://github.com/jmillikin/bazel-packages/blob/master/libevent/libevent.bzl for an example. The glob is on input files, and used to calculate the output file names using normal Python expressions.

Please use caps for shell vars like PROTOC, CONFIGURE here

CAPITAL_CASE variables are reserved for the OS or the shell. I'm using the Google Shell Style conventions here, which encourage lower_case names for variables defined by the user: https://google.github.io/styleguide/shell.xml#Naming_Conventions

Within Envoy, I think we have a slight variation where all variables are upper case. Where there is disagreement between Google style guide and Envoy, we tend to prefer consistency (unless you want to update this everywhere). Thanks.

I'd be fine updating this everywhere (might as well), if you're OK with the extra PRs and general commit churn.

Fine with me. @mattklein123?

Why is existing_rule_keys needed? Please add a comment if it's necessary

It lets users override the dependencies simply by defining their own, instead of having to adjust the params of envoy_dependencies. For example, I could have a WORKSPACE that uses a custom protobuf_bzl, and then this code would use it automatically.

I'll add a comment.

It seems crazy to have two mechanisms for this (this and the skip_targets). I don't feel that strongly on which is better, but we should just have one, even if we need to deprecate skip_targets.

I would prefer skip_targets, since there some optional dependencies that can be skipped without overriding, e.g. lightstep.

@htuch I'd like to deprecate skip_targets once the migration is finished, because it'll be redundant and is harder to use when Envoy is built as part of a larger workspace.

@lizan If a dependency is optional, then Bazel won't try to fetch it unless it's needed to build a target the user asked for.

@lizan can we get rid of skip_targets based on this? Would really prefer just one approach here.

ah you're right, I'm fine with that.

Does this work when the repo is consumed by other git_repository or local_repository? Make sure this works in envoy-filter-example, or istio/proxy.

We had to use build_file_content to embed them into .bzl files, just want to double check if things changed.

build_file takes a label, resolved relative to the current repository (bazelbuild/bazel#682). This will work when building Envoy as part of a larger workspace.

I believe older versions of Bazel (< 4.something?) used plain paths for build_file. If we need to support those versions, I can change to build_file_content. But I'd prefer not to, because it's ugly.

No I don't need older version support, Good to know it works now.

Do you even need empty.cc? I was under the impression that cc_library targets could be used to aggregate libraries without providing their own sources.

See the comment. An empty .cc file is needed to make Bazel trigger a build. A cc_library that depends only on .a files will report "nothing to do" if you try to build it on the command line.

This is quite neat, but also non-obvious to the reader. Can you add a comment on each of these sites where you reference GENRULE_REPOSITORY_COMMAND and explain where it is coming from?

Changed to use a function that takes a label. This should be a bit more self-describing.

Please add a comment on why this is needed, i.e. that you need the protobuf library headers/libraries to feed into GENRULE_REPOSITORY_COMMAND below.

Done.

Please either switch back to upper case or put in a PR first to adjust the entire repository to a lower case convention for internal variables. You can always re-lower case it later if you want to delay that repository conversion PR.

Changed to upper-case. I don't want to barrier this PR on some giant shell blob cleanup.

Maybe have the extensions of these .genrule.cmd or something, so that it can't be confused with real .sh.

Done.

@lizan can we get rid of skip_targets based on this? Would really prefer just one approach here.

Prefer 4 spaces here. We've been a bit inconsistent, but the idea is that this Skylark, not Python and we're trying to be consistent across BUILD files and .bzl.

Done. Also ran buildifier.

This approach is great, I like the idea of getting the build under genrule (for build parallelism) while still doing the setup heavy lifting under repository_rule. It doesn't solve the problem of having to explicitly enumerate output files in the genrule; so far, in this PR it's not an issue, but it's something I think we should consider as we bring in more dependencies with more complex output file lists.

I'm good with this approach, providing we maintain the bind support so that folks who really want prebuilts (e.g. that don't come from our build process), are capable of arbitrary rebinding in their WORKSPACE.

Rather than using bind(), we should encourage users to override deps using Bazel repositories. That's why the existing_rule_keys stuff is used -- a user can define a different @protobuf_bzl repository in their own WORKSPACE.

See comments on https://docs.bazel.build/versions/master/be/workspace.html#bind and discussion on bazelbuild/bazel#1952 ("Consider removing bind()") for more background on discouraging use of bind().

As long as we can support remapping of the external repos via envoy_exernal_dep_path, we're OK with this at Google. Internally, as you know, we don't have any @ repos, so we need to be able to take each of the individual repository/dependency labels and remap them to a global path in our monorepo.

What is envoy_exernal_dep_path? Is that the same as the path param to envoy_dependencies?

How do you currently handle the bind() names? I assume there's something set up to avoid polluting the namespace with things like //external:zlib -- do those get transparently mapped to //third_party/ somehow?

https://github.com/envoyproxy/envoy/blob/master/bazel/envoy_build_system.bzl#L71. Internally, we rewrite the entire envoy_build_system.bzl to match Google's monorepo and build requirements. That function gets turned into a dictionary lookup, that maps from identifiers such as ares to something in //third_party.

Ah, that should be fine then. This PR does make a small change to envoy_external_dep_path to support non-ambiguous dep names, but that's merely a change in the identifiers. And since none of them are changing in this PR, you don't have to do anything.

What's the Bazel story for aliasing without bind()? Suppose I have a repository under @foo, I import @bar, and it expects @foo to exists as @baz. How do I tell Bazel to find @baz as @foo?

Have you looked into the size of the Docker image? We're now doing 3x the build of external deps. We briefly went from 1 -> 2 and that created almost 50% growth. Please provide stats in the review and I think @mattklein123 might want to review this for sanity.

On master the image is 2.34GB, on this branch the image is 2.17GB. Bazel is fairly good at avoiding copies of files that aren't different between compilation modes.

I'm also preparing another PR to trim the image by reducing Debian deps with apt-get --no-install-recommends, and have filed a Bazel bug to allow use of the headless JDK because we don't need X11 (bazelbuild/bazel#3761).

Thanks, reducing image size is a nice improvement.

This is pretty complicated for the average Envoy developer who isn't a Bazel expert. A build recipe was easy to understand, it's just a shell script. Can you provide some simple explanations above imagining the developer has never heard of a genrule? Even a link to Bazel genrule stuff would help, but we don't want folks to have to become Bazel ninjas to add deps.

Made a few tweaks, does this help? I added a link to the genrule docs, and phrased it as a "bash command with Bazel extensions" which should hopefully be less intimidating to devs unfamiliar with Bazel.

Can you document how developers deal with external dependencies that depend on other external deps? It was a lot more direct before (we just has essentially a global prefix path).

I'm not sure how to document this, because it'll be different for each dep -- some use Autotools and thus need flags to ./configure, others use CMake and need ... whatever CMake does. I added a link to the Lightstep build command to use as an example.

Nit: inconsistent indentation.

Fixed.

What about repository_locations.bzl? This was explicitly added to allow site-local redirects of deps.

I would prefer to have one way to override dependencies, if possible. This implementation allows deps to be overridden by adding things to WORKSPACE, which lets us avoid making local changes to the Envoy source files.