bazel-contrib / bazel_features

Support Bazel "feature detection" from starlark
https://docs.google.com/document/d/1HJf3gMYIrzmTRqbD4nWXH2eJRHXjLrOU0mmIeZplUzY/edit#heading=h.5mcn15i0e1ch
Apache License 2.0
17 stars 13 forks source link

FR: bzl_library for bzl files #31

Closed rickeylev closed 7 months ago

rickeylev commented 9 months ago

This mostly make it easier to generate docs for projects that depend on bazel-features and use bzl_library for their bzl libraries, but is also generally useful for bzl files that are libraries and need their dependencies modeled.

Without such a feature, projects have to manually define a bzl_library target to include the sources, which is just boilerplate:

bzl_library(
    name = "bazel_features_bzl",
    srcs = ["@bazel_features//:bzl_files"],
)
fmeum commented 9 months ago

The original motivation for this was to keep the dependency footprint of this library as small as possible. We want to ensure that every ruleset can use this library and rely on it not ever breaking anything. The best way to achieve that is by not having any deps - not even bazel_skylib.

We might want to reconsider skylib as it is in a similar position, but ideally bzl_library would move either to Bazel itself or to some independent, equally lightweight module.

Wyverald commented 9 months ago

@brandjon is it still the plan to make bzl_library a native rule?

cgrindel commented 8 months ago

I just ran into this as well. Could we reconsider just adding a dep on Skylib for now. I agree that bzl_library should be part of Bazel. I created #33 adding the missing bzl_library targets and a test to ensure that they stay valid.

rickeylev commented 8 months ago

Just a thought.

As a user, the concern I have is: "is the srcs value I use for my bzl_library correct? I'm not sure. Maybe bazel_features splits things and I'll need add a deps. They aren't providing the actual target I need, so I'm not sure what guarantees I have about this."

Perhaps something that would help is a macro to indirect the creation? e.g.

load("@bazel_features//...", "bzl_library_maker")
load("@skylib//...", "bzl_library")

bzl_library_maker(name="bazel_features_bzl", bzl_library=bzl_library)

This is still boiler plate, but now I, as a user, don't have to worry about any bazel_features idiosyncrasies or bzl_library idiosyncrasies. If we wanted to take it a step further, I suppose bazel_features could generate a repo that carries a skylib dependency. This somewhat isolates the dependency, and only incurs it for doc generation (as opposed to all usages).

fmeum commented 8 months ago

Coming back to this, I wonder whether it may just be overthinking on my part. With Bzlmod @bazel_tools itself depends on skylib and WORKSPACE is going away, so an additional dep on skylib through bazel_features likely won't matter.

cgrindel commented 8 months ago

@rickeylev The bzl_test in #33 exists to ensure that the bzl library dependency graph is correct. If it is not, stardoc becomes very angry. 😡

cgrindel commented 8 months ago

@fmeum The placement of bzl_library in Skylib is awkward. If we can't move it to bazel_tools, perhaps we could put it in its own repo and have features depend on the new repo. 🤔

cgrindel commented 8 months ago

As an aside, I was thinking about creating a repo under bazel-contrib just for bzl_test. I have recreated this recipe in several repositories, at this point.