search

google / cel-go

Fast, portable, non-Turing complete expression evaluation with gradual typing (Go)
https://cel.dev
Apache License 2.0
2.16k stars 216 forks source link

Non-public bazel visibility for parser/gen #947

Open RamLavi opened 2 months ago

RamLavi commented 2 months ago

Feature request checklist

Change Please consider changing the parser/gen package's bazel visibility to //visibility:public.

parser/gen is not an internal package, so go build have no issue building code that uses the package, but parser/gen's BUILD.bazel sets the default_visibility to subpackages.

It looks like the package was made like this in the initial implementation.

The issue is that it causes error when trying to build code that uses parser/gen with bazel/gazelle, because parser/gen is not externally visible:

ERROR: /root/go/src/kubevirt.io/kubevirt/vendor/github.com/google/cel-go/parser/BUILD.bazel:7:11: no such target '//vendor/github.com/google/cel-go/parser/gen:go_default_library': target 'go_default_library' not declared in package 'vendor/github.com/google/cel-go/parser/gen' defined by /root/go/src/kubevirt.io/kubevirt/vendor/github.com/google/cel-go/parser/gen/BUILD.bazel and referenced by '//vendor/github.com/google/cel-go/parser:go_default_library'

Example I'm using this lovely repo in order to unit test ValidatingAdmissionPolicy I'm using. I'm importing this repo in order to implement the ExpressionAccessor interface.

Alternatives considered Not the I know of.

TristonianJones commented 2 months ago

@RamLavi You shouldn't have any direct dependencies on parser/gen, but I hear that vendoring seems to mess with gazelle. From the gazelle docs, I see we can try # gazelle:go_visibility label which should potentially allow you to override the vendored visibility. I'm not sure if I need to update this directive in the cel-go repo or if you can manage it from your side. Honestly, happy to try both.

RamLavi commented 2 months ago

Hey thanks for the prompt reply!

@RamLavi You shouldn't have any direct dependencies on parser/gen, but I hear that vendoring seems to mess with gazelle.

It is indeed a mystery to me, my import is only in order to implement an object that complies to the ExpressionAccessor interface, so it's not clear why this visibility issue is there in the first place. Moreover, this is not the only vendor we're importing that uses __subpackages__ visibility (github.com/onsi/ginkgo/v2 for example) and we never encountered this before.

From the gazelle docs, I see we can try # gazelle:go_visibility label which should potentially allow you to override the vendored visibility. I'm not sure if I need to update this directive in the cel-go repo or if you can manage it from your side. Honestly, happy to try both.

i think the # gazelle:go_visibility label directive needs to be done from the importing (=my) side, and I'm asking my team in order to see how they feel about it, but I also wanted to know - is there a reason for this folder to be non-public? My concern is that other repos that use bazel and try to write unit tests for validatingAdmissionPolicies may encounter similar visibility issues.

TristonianJones commented 2 months ago

The parser/gen package is truly internal and we don't want anyone to depend on it accidentally. I'm not sure if there's a better package layout which would avoid the vendoring visibility issue, but we should be able to move the package pretty easily if so.

RamLavi commented 2 months ago

The parser/gen package is truly internal and we don't want anyone to depend on it accidentally. I'm not sure if there's a better package layout which would avoid the vendoring visibility issue, but we should be able to move the package pretty easily if so.

ACK. I will try to dig some more on bazel logs and see if there's a clue why it happens and how we can work around it. Is that OK if we keep this issue open until we have more information? Thanks for the help!

TristonianJones commented 2 months ago

@RamLavi totally fine to keep the issue open until we know more. My best guess is that the visibility isn't happy with being moved to a new directory. From what I understand, this can be fixed at the tooling layer with annotations in BUILD files, but it's not a very friendly user experience either way

TristonianJones commented 2 months ago

@RamLavi any progress to report?

RamLavi commented 1 month ago

@RamLavi any progress to report?

BAZEL is hard, and not well documented :) I didn't manage to understand how to properly set the label so that it will ignore your library's visibility. I'm currently waiting for support from my team, but they are not yet free to assist on this matter.