Add ENABLE_BAZEL_PACKAGES_LOAD_HACK=true for poor Bazel users

[slimsag]

Hello /Sourcegraph employee hat on/ 👋 On behalf of Sourcegraph, we'd like to contribute these changes back to autogold that enable users of the Bazel build systems to make use of autogold despite a bug/disagreement in Go/Bazel (see below.) We agree to the project terms & license these changes under the open source LICENSE, as we don't want the annoyance of maintaining a fork of this code on our side long term.

---

Context: Bazel build systems try to keep hermeticity by setting PATH="." - but Go does not like this as it is a security concern; almost all Go tooling relies on golang.org/x/tools/go/packages.Load which behind the scenes must invoke go and uses the secure version of x/sys/execabs, but ultimately this means Go tools like autogold cannot be run in Bazel:

golang/go#57304

Autogold relies on packages.Load in order to determine the Go package name / path when writing out a Go AST representation of the value passed in; but the issue above means autogold cannot be used with Bazel without removing "." from your PATH, which Bazel claims breaks hermeticity (one of the whole reasons people use Bazel.)

For Bazel users, we allow them to set ENABLE_BAZEL_HACKS=true which causes autogold to guess/infer package names and paths using stack trace information and import paths. This is not perfect, it doesn't respect packages whose import paths donot match their defined package foo statement for example - but it's sufficient to enable autogold to be used in Bazel build environments where the above Go/Bazel bug is found.

• By selecting this checkbox, I agree to license my contributions to this project under the license(s) described in the LICENSE file, and I have the right to do so or have received permission to do so by an employer or client I am producing work for whom has this right.

[codecov-commenter]

Codecov Report

Merging #40 (4c87f57) into main (bae639c) will decrease coverage by 2.72%.
The diff coverage is 5.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main      #40      +/-   ##
==========================================
- Coverage   35.26%   32.54%   -2.72%     
==========================================
  Files           5        6       +1     
  Lines         346      381      +35     
==========================================
+ Hits          122      124       +2     
- Misses        198      229      +31     
- Partials       26       28       +2     

Impacted Files	Coverage Δ
diff.go	64.00% <0.00%> (-5.57%)	⬇️
expect.go	36.41% <0.00%> (-0.38%)	⬇️
bazel.go	6.45% <6.45%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[jhchabran]

I confirm it's working after testing things on our side. The thing we're wondering about is that if we should name that variable BAZEL_TEST=1 which would by default enable this behaviour. That being said, the fact that this would be implicit (because Bazel sets that variable to 1 on its own) might be a problem.

Another solution would be to rename it to ENABLE_AUTOGOLD_BAZEL_HACKS so anyone reading the Buildfile would understand which package is affected by the env var.

[slimsag]

Gotcha, cool, I'll get this merged soon - as for env var naming, I consider the following in naming this:

1. Ultimately, this is a bug in Go packages.Load or in Bazel - not autogold; merging these hacks here really isn't correct but I'm doing so to be kind to our bazel users :) these hacks have drawbacks though, the correct fix is in packages.Load or Bazel ultimately. One should know if they are using hacks, and so it shouldn't be enabled by default / implicitly.
2. Given that pretty much every Go tool out there uses packages.Load, it seems likely others will run into this with other non-autogold tooling; in such a case it'd be nice to not have to specify one env var for each tool you use, if possible.
3. It should convey it is Bazel-specific (contains BAZEL)

Thus I come up with:

• Must contain HACK
• Must contain BAZEL
• Should not contain AUTOGOLD
• May contain GO, PACKAGES, LOAD

I can see how ENABLE_BAZEL_HACKS might appear as a statement about my perception of Bazel though ;) How's ENABLE_BAZEL_PACKAGES_LOAD_HACK instead in all seriousness?

Separately, if we do want a different name (or on-by-default behavior) in sourcegraph we're totally free to fork (really, I don't mind that at all so if that makes sense for us there that works for me.)

[davejrt]

This totally makes sense. I don't believe we'll have any issue with using ENABLE_BAZEL_PACKAGES_LOAD_HACK as the name as we can document this reflecting your thoughts above to ensure it's well communicated this issue is further upstream.

[jhchabran]

I'm totally fine with ENABLE_BAZEL_PACKAGES_LOAD_HACK, it conveys enough context, that's perfect.

Ultimately, this is a bug in Go packages.Load or in Bazel - not autogold;

We all agree on that. We're seeing conflicting design goals here and it's great that we can compensate that on autogold's side.

Ultimately, this is a bug in Go packages.Load or in Bazel

Yes, execabs clashes with how Bazel expects things to be, but I have very little hope that this would be fixed soon, it's pretty niche.

Given that pretty much every Go tool out there uses packages.Load, it seems likely others will run into this with other non-autogold tooling; in such a case it'd be nice to not have to specify one env var for each tool you use, if possible.

Makes sense, thanks.