Explicit outputs for wasm_cc_binary

[zaucy]

I originally started this PR because I was running into something similar to #986 and found out that my archive was libfoo.so and not the expected foo.so wasm_binary.py required. This PR doesn't just solve that problem for me, but it also allows me to specify the name of the outputs as well as which outputs I would like.

wasm_cc_binary(
    name = "foo",
    cc_target = ":foo_cc",
    # NEW
    outputs = [
        "foo.wasm",
        "foo.js",
    ],
)

I think this is a good change for 2 reasons:

1. rule outputs atttribute is deprecated
2. Outputs are more strict. If I want a sepcific output and my configuration doesn't allow that output, I get an error.

Let me know what you think!

[walkingeyerobot]

Thanks for the PR! I'm all for moving away from deprecated bazel APIs :)

I'd like to learn more about what exactly is allowed by the new outputs parameter. What happens if I specify ['foo.wasm', 'bar.js']? I imagine this would fail during link in wasm_binary.py because bar.js does not exist? If this is the case, I'd prefer to fail earlier, i.e. in the rule, so we don't waste time doing a full build that's not going to work.

It's also noteworthy that I believe we can't just specify extensions in an outputs attribute because those would not be known to bazel ahead of time and therefore wouldn't be able to be used by rules that consume wasm_cc_binary files (although it would work if users were using wasm_cc_binary as their final rule).

[walkingeyerobot]

Is there a way to set this to True instead? We'd rather not break existing users.

[walkingeyerobot]

Ok we might be able to do something with a macro that sits in front of wasm_cc_binary and sets outputs to a default list if it's not set, effectively keeping the behavior. I am happy to help with this part as it might be a bit tricky.

The big thing here is I don't want to break an unknown number of projects depending on this rule with a significant breaking API change.

[zaucy]

I added a default outputs when the outputs attribute is empty. That does mean that #986 would still be broken (maybe?) when the outputs are empty.

[walkingeyerobot]

Sadly this is still broken. To reproduce, create a new folder next to hello-world called "foo", add a BUILD file with these contents:

filegroup(
    name = "foo",
    srcs = ["//hello-world:hello-world.wasm"],
)

Because hello-world.wasm is not an explicit output, the build fails. If you add it under the outputs attribute, this works (although you might need to mess with visibility or pass --nocheck_visibility).

[zaucy]

Ah I see. Unfortunately simply wrapping the rule in a macro for default outputs wouldn't work either since currently on the main branch the outputs depend on the cc_target's name, not just the wasm_cc_binary's name.

If the rule was wrapped in a macro and it's okay to break the default output paths not considering cc_targets name then that's great! Otherwise there could be two internal rules that the macro chooses from.

I'm capable of doing either option. If you have a preference let me know and I'll make the necessary changes.

[walkingeyerobot]

You're right, this is more tricky than I initially thought.

I think we should parse the cc_target label in the macro to keep existing behavior as much as possible. Inside the macro, you should just be able to treat the cc_target as a string. I acknowledge that if someone is select()ing on their cc_target this will break as the select()s are not resolved when the macro is resolving. I don't see a way to avoid breaking that, but I don't believe the number of users affected would be very high.

I still believe this change is quite valuable overall. Thank you for your patience and effort here.

[zaucy]

I went ahead and created a macro that decides to use either a "legacy" rule or the new rule. I thought that might be better than parsing the cc_target label in the macro. ce1cea7

[zaucy]

I'd like to learn more about what exactly is allowed by the new outputs parameter. What happens if I specify ['foo.wasm', 'bar.js']? I imagine this would fail during link in wasm_binary.py because bar.js does not exist?

Ah yes that would fail. My main usecase has been standalone wasm so I'm a little out of touch with the other output files. Would checking for certain output filenames matching be scalable? Or is that a painful maintenance burden? I don't know the details for which files are required with which settings.

It's also noteworthy that I believe we can't just specify extensions in an outputs attribute because those would not be known to bazel ahead of time and therefore wouldn't be able to be used by rules that consume wasm_cc_binary files

The output paths (and extname) are known in the rule with ctx.outputs.outputs. Would creating a Provider for each type of output (or maybe groups of outputs?) be flexible enough for rules consuming the wasm_cc_binary files?

Alternatively since the outputs are explicit they could also explicitly be passed into another rule, correct?

[walkingeyerobot]

I'd like to learn more about what exactly is allowed by the new outputs parameter. What happens if I specify ['foo.wasm', 'bar.js']? I imagine this would fail during link in wasm_binary.py because bar.js does not exist?

Ah yes that would fail. My main usecase has been standalone wasm so I'm a little out of touch with the other output files. Would checking for certain output filenames matching be scalable? Or is that a painful maintenance burden? I don't know the details for which files are required with which settings.

It's also noteworthy that I believe we can't just specify extensions in an outputs attribute because those would not be known to bazel ahead of time and therefore wouldn't be able to be used by rules that consume wasm_cc_binary files

The output paths (and extname) are known in the rule with ctx.outputs.outputs. Would creating a Provider for each type of output (or maybe groups of outputs?) be flexible enough for rules consuming the wasm_cc_binary files?

Alternatively since the outputs are explicit they could also explicitly be passed into another rule, correct?

I'm not sure exactly how providers would work here. There is a solution that I'm confident will work. We can go back to disallowing outputs to be empty and add a macro in front of wasm_cc_binary that detects that outputs is empty and passes the full list there. That would make the outputs explicit if the user doesn't provide any.

I'll ask some people (@trybka) to see if they know more about how providers might help us here.

[trybka]

The output paths (and extname) are known in the rule with ctx.outputs.outputs. Would creating a Provider for each type of output (or maybe groups of outputs?) be flexible enough for rules consuming the wasm_cc_binary files?

They are only known if specifying the attribute (i.e. non-empty)--this doesn't help with supporting pre-existing legacy usecase(s) where folks are just depending on these implicit outputs existing.

e.g. you could do the following (and people do this):

genrule(
    name = "compile_js",
    srcs = ["//mywasm:foo.js", "mycustomfooter.js"],
    outs = ["compiled.js"],
    cmd = ["cat $(SRCS) > $(OUTS)"]
)

Providers for specific use-cases are a fine extension to add for, e.g. js_library, but I don't think they help here.

OutputGroupInfo is a provider specifically for doing different file outputs, but still doesn't solve our pre-declared output file problem, I'm afraid.

Alternatively since the outputs are explicit they could also explicitly be passed into another rule, correct?

Yes. See above.

[trybka]

can you set name = "wasm_cc_binary" for both of these?

that way the target type matches for things that care about the rule type.

[zaucy]

Done! I didn't know that existed. Thanks!

I read the doc about it, but I have two questions if you don't mind answering:

1. Is it not a problem that both _wasm_cc_binary_legacy and _wasm_cc_binary have the same name?
2. Is it recommended that the rule name attribute is set for any rule that is wrapped in this style? I'd like to update my internal rules if that's the case. I'm unsure what the consequences of not doing it is.

[trybka]

1. This is basically the only use for the name parameter AFAIK--when you need to do something spicy like this.
   See also: cc_test.bzl's usage

2. I dunno if it's "recommended" or not, but when you care about the rule type being the same (as, in this case, we do -- for things like bazel query "kind(wasm_cc_binary...)" or native.existing_rules), you have two options:
   a) either use name or
   b) do some file import dance like cc_binary does. cc_binary does this thing where it defines the rules in separate files so they can both be named cc_binary, but then imports them with a different name to do this macro wrap.

I personally prefer using name, but I suppose both are correct so it might just be a matter of style.

[zaucy]

I've cherry picked #1049 to get past the macOS issue (I can remove if this PR gets approved) and I also rebased onto 891b449 because the latest on main (93f21c9) linux bazel is failing.

[walkingeyerobot]

This looks good to me! I think I'd like to get the bazelisk changed merged before merging this, but other than that it looks good to go.

Thanks very much for your patience and hard work on this!