Injecting module state inside BUILD files?

[facebook]

Injecting module state inside BUILD files?

[ndmitchell]

To do that today we'd suggest defining your own rust_binary in a .bzl file, which imports the prelude one and sets a few extra fields before calling on, e.g. https://github.com/facebookincubator/buck2/blob/63cdb4872bf19cb7a6651cdf951b4acff6ee1bf6/shim/shims.bzl#L30-L43.

When we first introduced the oncall function myself and @stepancheg discussed whether to generalize this to support arbitrary attributes. There was some timeliness issues on our side, so we went for the simplest possible thing that could be extended later. Note that oncall adds an annotation that is not used in building, which means it has less issues than something like license, as you might expect rust_binary to have a license, but not a genrule.

[thoughtpolice]

So right now the field isn't used for building, but I want to analyze it with BXL in a separate step (e.g. to check license compatibility). And so, yes, it only makes sense on certain rules. An annotation that applies to all rules isn't necessarily wrong — you could just exclude certain classes of rules or whitelist them, which is probably good — but that's a point worth keeping it in mind.

So, maybe another way of phrasing it is something like: I want a "license": attrs.string attribute on a rule to default to a global value, if the license() field is filled out. But it could also be specified manually, too.

Anyway, I suspect this is something that can't be immediately generalized, but I think some kind of "declarative specification of target metadata" idea we're getting at here would be good. You can imagine something like oncall() or author() or license() and all kinds of stuff that you could attach to rules to analyze later.

[ndmitchell]

Could you just define:

def license(x):
    oncall(x)

And then reuse the oncall stuff? A bit of a hack, but should give you what you need now.

[thoughtpolice]

I can read the source a bit more but that might be good — is oncall exposed as a property on the subsequent rule or something that I can query from an analysis context or provider in BXL? If so that would work, though it doesn't work for more than 1 thing.

I was also wondering if I could do something like this:

def license(x):
    load_symbols({ '_sneaky_license_symbol': x })

and then somehow refer to that symbol in the rule definition, but that's not quite enough, since I don't think load_symbols works the way I expect inside a BUILD file...

[ndmitchell]

You can't use load_symbols. But it wouldn't be too hard to add a set_global/get_global function. Only worry is that if you add it, then people might abuse it.

For oncall, you can get them as the attribute buck.oncall I believe.

[akrieger]

I do not like the idea of mutable or even write-once globals (which are still 'mutable' because their initial value is None).

Right now, targets files have the advantage that the order of execution is largely irrelevant. You can hoist all loads to the top level and execute statements in largely an arbitrary order, and the rule calls you get at the end will be the same. You can't accidentally change the graph by reordering calls. This appeals to me, as someone who has to frequently edit and refactor unfamiliar targets files, because it gives me more confidence that I'm doing exactly what I think I'm doing and nothing else. The same is not true with any kind of globals mechanism.

load(":defs.bzl", "define_targets")

cxx_library(name = "first")

define_targets()

cxx_library(name = "second") # define_targets() could change this arbitrarily

Lets says someone refactors it to move the define_targets() call up or down. Without realizing it, they've just changed the output of first and second. You could argue that 'oh its such a small file someone or reviewers would know', and I could counter with 'this doesn't scale past two pages of text'.

This also opens us up to spooky action at a distance. There's basically no way to know how this will evaluate, ever, without expressly performing the evaluation to see:

# is the library alive, or dead?
cxx_library(name = "heisebergs_cat", srcs = glob(["dead/**/*" if get_global("radioactive") else "alive/**/*"]))

The concerns above can be mitigated somewhat by simply disallowing accessing (or at least setting) globals within bzl files. But if we're going to do that, then I'm inclined to say that true read only package level configs are the right way to go. Essentially a buckconfig file you can have inside any folder with an alternate access api to read_config.

[thoughtpolice]

Yeah, I agree even write-once variables are a bit of a nuisance. They make semantics awful and in otherwise purely evaluated code tend to crop up as problems. Now, I won't say they're mandatory or anything in the real world — obviously they aren't for y'all — but I wonder if the tide will hold on that particular thing forever (for any potential feature X, someone else will probably demand it and say it is mandatory)...

But I want to clarify one thing: the whole thing with globals? That's just me fucking around with the API to see if things could work. That's just incidental. But I don't need a global setting or variable to be written, and I don't even need to modify anything in the graph in this case! No rules use this information, just BXL. In fact, the only thing I think I need is just the ability to look at package-level "attributes" like this with BXL. No globals necessary, just the ability to say "The package foo//bar/baz has a license attribute set on it." It is purely a synthetic construct so that I can analyze things more easily. A license field on every rule works too, of course, as I said originally.

To be clear my main desire here is that I want to do a license compatibility/audit check on packages and rule outputs, so that e.g. a rule that has license = "GPL-3.0" can be flagged if it is a dependency of a rule with license = "BSD-2". This feels like something perfect for BXL (e.g. I don't need it evaluated or put it inside an artifact.) Having the ability to say "All rules in this package have this field applied" and to be able to analyze a package/target and get the fields (which I can already do with .license fields) is all I want, it's just a easier to read and write than adding an attribute to literally every rule.

But if we're going to do that, then I'm inclined to say that true read only package level configs are the right way to go. Essentially a buckconfig file you can have inside any folder with an alternate access api to read_config.

Yes, pretty much that's what I want! Just a way to look at purely read-only, static data, that exists for a package. It's just that putting that inside the BUILD file seems somewhat natural for readability provenance reasons; it's easier to read everything there at one time rather than adding files for every little thing like this.

Having said that, the title of this ticket is now very misleading :)

[mzlee]

There is a concept of a PACKAGE file in buck1 to set attributes package-wide (https://github.com/facebook/buck/blob/main/src/com/facebook/buck/core/model/targetgraph/impl/Package.java). However, I don't see an implementation in buck2 yet. The buck1 implementation only allowed setting visibility, but I don't see why adding license would be a problem (assuming PACKAGE files survive and ported to buck2).

[thoughtpolice]

So PACKAGE files are not only coming back, they're already here! See the RFC and 5f717b8.

Naturally, I ported my little example repo over within the hour of them coming out. :) This is almost perfectly satisfactory for (probably all?) of my needs, except that:

• I can't query package values via cquery or some other mechanism, so no BXL support either

The main option I tried was attaching a package value to a rule attribute, through defaults. But because read_package_value is only available in BUILD files, I also can't have it as the default_only value for an string attribute. I haven't tried using a macro yet, though.

[thoughtpolice]

So I think this can be considered "done" for me. At least for now. As a matter of record, here's where I'm at, regarding the original example. With PACKAGE file support, I've been able to successfully abstract out some common metadata properties and then have rules which (when written correctly) have those properties applied as attributes. So then cquery works fine.

Let's assume we have some APIs to use for PACKAGE files:

__VALID_OWNERS = [
    "@aseipp"
]

def owner(name: "string") -> "NoneType":
    """Set the owner of the current package."""

    if name not in __VALID_OWNERS:
        fail(
            """Invalid owner: {}

            The list of valid owners can be seen in @prelude//basics/pkg.bzl
            """.format(name)
        )

    return write_package_value('meta.owner', name)

def license(s: "string") -> "NoneType":
    """Set the license of the current package."""
    return write_package_value('meta.license', s.strip())

def description(s: "string") -> "NoneType":
    """Set the description of the current package."""
    return write_package_value('meta.description', s.strip())

def version(s: "string") -> "NoneType":
    """Set the version of the current package."""
    return write_package_value('meta.version', s.strip())

Now have a PACKAGE file:

owner("@aseipp")
license("MIT OR Apache-2.0")

---

To drag this metadata along with a rule, you can use two higher-order helpers to abstract rule() assignments and rule applications:

def __rule_with_metadata(**kwargs):
    """A wrapper around `rule()` that adds metadata to the rule as part of its attributes."""
    kwargs['attrs'] = kwargs['attrs'] | {
        "__meta_owner": attrs.string(),
        "__meta_license": attrs.string(),
        "__meta_description": attrs.option(attrs.string(), default = None),
        "__meta_version": attrs.option(attrs.string(), default = None),
    }
    return rule(**kwargs)

def __rule_apply_metadata(fn):
    """A wrapper around a rule function that applies metadata from the local PACKAGE to the rule."""
    def k(**kwargs):
        fn(
            __meta_license = read_package_value('meta.license'),
            __meta_owner = read_package_value('meta.owner'),
            __meta_description = read_package_value('meta.description'),
            __meta_version = read_package_value('meta.version'),
            **kwargs
        )
    return k

Now, define a rule and assign it, then export a wrapped version (has to be done in two steps, because rule() has to be assigned to a top level value):

__my_rule_name = rule_with_metadata(
    impl = ...,
    attrs = { ... },
)

my_rule_name = rule_apply_metadata(__my_rule_name)

You can then cquery anything using __my_rule_name (cf #95) and look at the resolved __meta_* attributes. This setup keeps the metadata attached to rules abstracted from the rule definition, so you can have a single place to keep everything in sync.

This is overall much, much nicer than the original example of abusing oncall(), so I think this can be closed. If I have more targeted requests I'll put them in other tickets!