Non-public bazel visibility for parser/gen

[RamLavi]

Feature request checklist

• There are no issues that match the desired change
• The change is large enough it can't be addressed with a simple Pull Request
• If this is a bug, please file a Bug Report.

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]

@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]

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]

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]

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]

@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]

@RamLavi any progress to report?

[RamLavi]

@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.

[SlayerBirden]

Hi, I wanted to just emphasise that this library is essentially broken for anyone using Bazel. This bug is not getting a lot of traction because I think the majority of users aren't using Bazel.

The first example on this page https://pkg.go.dev/github.com/google/cel-go/cel#pkg-overview is not possible since github.com/google/cel-go/cel package depends on parser: https://github.com/google/cel-go/blob/master/cel/BUILD.bazel#L41, and parser depends on gen: https://github.com/google/cel-go/blob/master/parser/BUILD.bazel#L28 which is not public.

As far as I understand maintainers don't want to make gen public? But what is the alternative? How can private package be a dependency of the public one?

@TristonianJones

[TristonianJones]

@SlayerBirden bazel dependency graphs aren't supposed to be all set to the same visibility in order to function properly; however, I don't think that the bazel team considered go vendoring. I will poke around to see what the options are, but often such interactions are non-trivial and poorly documented

[SlayerBirden]

@TristonianJones thanks for the update! If you have any suggestion I'd also appreciate that.

[RamLavi]

@TristonianJones thanks for looking into this! Can you share if you have any update?

[TristonianJones]

@RamLavi I'm hopeful the change is as simple as what's been put forward for review.