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

[feature request] Consider moving bzl_library.bzl into @bazel_tools #18391

Open
jmillikin opened this issue May 14, 2023 · 11 comments
Open

[feature request] Consider moving bzl_library.bzl into @bazel_tools #18391

jmillikin opened this issue May 14, 2023 · 11 comments
Labels
P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) stale Issues or PRs that are stale (no activity for 30 days) team-Rules-API API for writing rules/aspects: providers, runfiles, actions, artifacts type: feature request

Comments

@jmillikin
Copy link
Contributor

jmillikin commented May 14, 2023

Description of the feature request:

Skylib's bzl_library.bzl provides a StarlarkLibraryInfo provider and bzl_library rule, which are used for aggregating .bzl rules files for use by other analysis tools.

The definition of that provider and rule are quite stable (basically unchanged since 2018), and it has no dependencies on any other part of skylib.

The natural way to use that file is to load it from every BUILD file in a ruleset, like this:

# rules_example/example/BUILD
load("@bazel_skylib//:bzl_library.bzl", "bzl_library")

bzl_library(
    name = "bzl_srcs",
    srcs = ["defs.bzl", "repository.bzl", "toolchain.bzl"],
    deps = ["//example/internal:bzl_srcs"],
)

However, since the load needs to fetch skylib before evaluation continues, this means that skylib becomes a hard dependency for every client of the ruleset.

Given that the bzl_library.bzl file is generic, broadly useful, and very stable, could it be bundled into @bazel_tools instead?

@Pavank1992 Pavank1992 added the team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. label May 15, 2023
@meteorcloudy meteorcloudy added team-Loading-API BUILD file and macro processing: labels, package(), visibility, glob team-Rules-API API for writing rules/aspects: providers, runfiles, actions, artifacts and removed team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. team-Loading-API BUILD file and macro processing: labels, package(), visibility, glob labels May 16, 2023
@comius
Copy link
Contributor

comius commented May 16, 2023

cc @aiuto @brandjon
This was discussed before. What were the conclusion, should we do it?

@comius
Copy link
Contributor

comius commented May 16, 2023

@brandjon please triage

@aiuto
Copy link
Contributor

aiuto commented May 16, 2023

IIRC, the discussion just sort of faded away.

I would like to seeStarlarkLibraryInfo and maybe bzl_library shipped with Bazel. But I also want the ability to replace the implementation of each with a repository local one. While trying to create another ultra low-level part of the stack (rules_license), what I have found that I want is the ability to locally override individual files in @bazel_tools. For example, I want to be able to replace http_archive at the workspace level, even if others are loading from @bazel_tools

Currently I am at a split opinion here. StarlarkLibraryInfo is a fundamental constant, because Stardoc breaks unless everyone depends on the same instance of it, so moving it to @bazel_tools does seem right (in spite of our desire to prune tools). bzl_library I would like to be able to override locally without having to vendor in all skylib.

But writing this helped me clarify a thought. What I want to be able to declare is a workspace wide replacement for the path of any load(). That way I could create my own http_archive or bzl_library without having to completely replace skylib or tools.

@fmeum
Copy link
Collaborator

fmeum commented May 16, 2023

Instead of moving bzl_library into bazel_tools, how about moving it into a separate repo that doesn't contain anything else and just remains lightweight forever?

Skylib could add this repo to its dependencies and continue to forward the provider for backwards compatibility.

@jmillikin
Copy link
Contributor Author

skylib itself is not large (36 KB); the main issue is that it's a load-time dependency that all downstream users of a ruleset must declare. It's the transitive mandatory book-keeping that's a problem.

Maybe the answer is to have more bazel_tools-ish repos that get bundled into Bazel, but are designed to be modular and user-replaceable? You could imagine something like this:

load("@bazel_tools2_starlark//:bzl_library.bzl", "bzl_library")

where github.com/bazelbuild/bazel-tools2 is a repo, it contains files `starlark/{WORKSPACE, bzl_library.bzl}``, and it behaves the same as every other external repository except for having a "default" version bundled in with Bazel.

(The same could maybe be done with http_archive and friends)

@aiuto
Copy link
Contributor

aiuto commented May 19, 2023

Default versions of built in small repositories would help with rules_license as well. Eventually, everything depends on @rules_license//rules:license.bzl. It is a pain to include everywhere, so having it built in is a nice convenience. But it must be user replacable for those who want to mod the behavior.

@brandjon
Copy link
Member

See new proposal for a native bzl_library rule, and discussion here.

@comius comius added P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) and removed untriaged labels Aug 22, 2023
Copy link

Thank you for contributing to the Bazel repository! This issue has been marked as stale since it has not had any activity in the last 1+ years. It will be closed in the next 90 days unless any other activity occurs. If you think this issue is still relevant and should stay open, please post any comment here and the issue will no longer be marked as stale.

@github-actions github-actions bot added the stale Issues or PRs that are stale (no activity for 30 days) label Oct 26, 2024
@aiuto
Copy link
Contributor

aiuto commented Oct 26, 2024 via email

@jmillikin
Copy link
Contributor Author

jmillikin commented Oct 26, 2024

This is a super-ugly hack, but would it be possible to use getattr to break the dependency loop?

# bazel-skylib/internal/starlark_library_info.bzl
StarlarkLibraryInfo = provider(
    "Information on contained Starlark rules.",
    fields = {
        "srcs": "Top level rules files.",
        "transitive_srcs": "Transitive closure of rules files required for " +
                           "interpretation of the srcs",
    },
)

# bazel-skylib/bzl_library.bzl
load("@bazel_tools//:something_that_already_exists.bzl", "symbol_that_already_exists")
load("//internal:starlark_library_info.bzl", _StarlarkLibraryInfo = StarlarkLibraryInfo)

# Using a symbol in @bazel_tools that already exists in older Bazel versions (so the load succeeds),
# re-export its StarlarkLibraryInfo from skylib if present.
StarlarkLibraryInfo = getattr(symbol_that_already_exists, "StarlarkLibraryInfo", _StarlarkLibraryInfo)

# Alternative: add StarlarkLibraryInfo as builtin, which avoids the need to find
# an already-existing part of @bazel_tools to add on to.
StarlarkLibraryInfo = getattr(native, "StarlarkLibraryInfo", _StarlarkLibraryInfo)

The problem isn't the dependency mesh exactly, it's avoiding a mandatory minimum Bazel version bump for skylib, which means there needs to be some way to discover whether the current Bazel version has StarlarkLibraryInfo before trying to load() something that would fail on older Bazels.

@fmeum
Copy link
Collaborator

fmeum commented Oct 26, 2024

We have this machinery already in bazel_features: https://github.com/bazel-contrib/bazel_features/blob/main/private/globals.bzl

However, for something as foundational as skylib, adding new WORKSPACE deps may be prohibitively breaking. Once we could assume that everyone is on Bzlmod (even with Bazel 6), the migration would be really easy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) stale Issues or PRs that are stale (no activity for 30 days) team-Rules-API API for writing rules/aspects: providers, runfiles, actions, artifacts type: feature request
Projects
None yet
Development

No branches or pull requests

8 participants