bazel-contrib / rules_oci

Bazel rules for building OCI containers
Apache License 2.0
258 stars 133 forks source link

Any plans to add providers for writing rules that build on-top of rules_oci? #279

Open ensonic opened 1 year ago

ensonic commented 1 year ago

We're trying to port from rules_docker to rules_oci (see https://github.com/googlecloudrobotics/core/issues/130), but have trouble finding a good replacement for:

load("@io_bazel_rules_docker//container:providers.bzl", "ImageInfo", "ImportInfo") see https://github.com/googlecloudrobotics/core/blob/main/bazel/app_chart.bzl#L1

thesayyn commented 1 year ago

Usually, our response to "adding something new" to the ruleset is why do you think you need it to accomplish what you are trying to do?

ensonic commented 1 year ago

@thesayyn In order to port our bazel rule, we need access to the digest. From https://github.com/bazel-contrib/rules_oci/blob/main/oci/private/image.bzl#L172 it looks like there are no additonal outputs to obtain it.

thesayyn commented 1 year ago

Is this what would you like to obtain? https://github.com/bazel-contrib/rules_oci/issues/60

alexeagle commented 1 year ago

I think we should add an OutputGroupInfo so you can request the "digest" output file, but we wouldn't have to create any new Provider types to satisfy this FR. Sound right?

ensonic commented 1 year ago

Is this what would you like to obtain? #60

Thanks! That seems to work for now.

thesayyn commented 10 months ago

Does #346 fix this for you?

gzm0 commented 9 months ago

Maybe I can chime in my experience here: The additional label with the digest file is going to be very hard to compose for downstream rules (need to recalculate a label, so forced to use a macro, not configurable, etc.).

So far, I've just resorted to read the oci-layout index.json directly. This is through a well specified standard. Downsides are:

thesayyn commented 9 months ago
  • Needs validation to check the assumption that there is only one manifest in the output (oci-layout allows for more).

oci_* rules will always have a single image in the index.json so that's not really a concern here.

  • Needs some json processing

I believe it could be gathered with a simple sed command given that there's a single digest property.

gzm0 commented 9 months ago

That is correct, but only under the assumption that users always pass the correct rules forward: Without a provider, all there is to validate at analysis time is that the output is a single tree artifact.

For any more serious ruleset, I would not be comfortable just assuming the build user has wired everything correctly (for comparison: with a provider, I only need to trust the rule implementor).

gzm0 commented 9 months ago

Another datapoint: Turns out I needed the config digest, not the manifest digest. IIUC with the OIC layout it is infeasible to predict where it is going to end up (since it is content addressable). So to use it, the call-site needs to handle OIC layout anyways.

thesayyn commented 9 months ago

I don't see the point of adding a provider, just for the sake of validation. Adding a provider is the wrong tool for validation. A custom rule can still produce an invalid oci-layout and add the provider.

And this is separate concern than adding a provider to rules_oci. That'll probably solved via https://bazel.build/extending/rules#validation_actions. We are just waiting to drop bazel 5 support.

alexeagle commented 8 months ago

We could add a validation action now, and just create it conditional on using Bazel 6. I think this is now a different feature request?

gzm0 commented 8 months ago

I think this is now a different feature request?

Agreed.

Regarding provider and validation, I think I wasn't very clear. Apologies. I'll try to elaborate.

Consider the following, broken build.

create_dir(
  # some rule that creates an output directory.
  name = "foo",
  ...
)

oci_image(
  # some arbitrary image.
  name = "bar",
  ...
)

my_rule_using_an_image(
  name = "baz",
  image = "foo", # this should be bar
)

The user made a mistake here and passed the wrong label/target to my_rule_using_an_image.

I was looking at this situation from the perspective of the implementer of my_rule_using_an_image. IIUC, the best that can be done to give feedback to the user changes quite a bit with and without a provider:

With a provider

In the analysis phase, bazel itself will not accept the foo label since the target it refers to doesn't have the appropriate provider. The user will get a message to this extent. Hopefully:

Without a provider

Analysis phase

my_rule_using_an_image can check that image refers to a single tree artifact. This would not actually lead to an error in this example (assuming create_dir does what it says).

Execution phase

my_rule_using_an_image can check that the contents of the directory claim to be an OCI layout (check oci-layout and its contents) . This is highly likely to fail for any other directory (produced by an arbitrary rule).

While this gives the user appropriate feedback:

Discussion

If we look at the two scenarios above in the context of simply retrieving the manifest digest, in the second scenario (without a provider), the code required for the retrieval will be overshadowed by the code required to prevent users from making mistakes (which IMHO is paramount).

I guess my informal use of the term "validate" has caused some confusion. IIUC to solve the issue I've outlined, a validation action is not useful (or I have misunderstood something about validation actions :shrug: ).

Whether or not all of this is sufficient reason to actually add a provider to rules_oci, and/or whether a provider is the right tool to solve the outlined issue is not 100% clear to me. I just wanted to make sure we're on the same page RE the use cases I had in mind.

thesayyn commented 1 month ago

Validation actions do what providers would do for "validation" but at runtime. I'd also argue that adding a provider to get bazel to yell users with a message that looks like "target :x does not provide OCIImage provider." is not as useful as one would think.

Adding a provider for this use would simply prevent anyone from using things like genrule, run_binary. Here's an example i made a year later and it still stupidly simple because we don't have to deal with providers. https://github.com/bazel-contrib/rules_oci/pull/570/files#diff-e5546d9d1736e2980c4c365d05d59dcad35851551fa40ded3e24cc1c00049ab4R17

If we added providers for this, it would simply prevented us from doing things like this.

gzm0 commented 1 month ago

Here's an example i made a year later and it still stupidly simple because we don't have to deal with providers.

That's an excellent example indeed.

I guess this basically boils down to the nominal / structural subtyping discussion and to some extent the static / dynamic subtyping discussion.

In any case, it feels like at this point, consistency with the bazel ecosystem is significantly more important than doing "the right thing". I do not feel I have a sufficiently large understanding of how the ecosystem handles this to have any opinion TBH. So from my personal POV, feel free to close this issue.

thesayyn commented 1 month ago

You are correct, but this isn't because we are trying specifically do the right thing thus diverging from the community. You can find plenty of rulesets that don't have a custom provider.

oci_image is in sense is like pkg_tar, you feed it a bunch files and it packages them into a folder (as opposed to a tar file) that is understood by most container tooling.

I guess this basically boils down to the nominal / structural subtyping discussion and to some extent the static / dynamic subtyping discussion.

I agree.