Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding an initial bazel configuration #3141

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

CerealBoy
Copy link
Contributor

Description

This PR is to add Bazel configuration files to this repository that will allow the outputs of the repository to be constructed and managed (optionally) via Bazel.

Changes

Existing functionality is unchanged, beyond some additions to .gitignore. This PR is additive to the existing files at various path depths, they can be ignored under standard normal circumstances.

Testing

  • Tests have run locally (with go test ./...). Buildkite employees may check this if the pipeline has run automatically.
  • Code is formatted (with go fmt ./...)

Copy link
Contributor

@DrJosh9000 DrJosh9000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great! I haven't looked closely at what I assume gazelle generated, but seems fine.

The main hurdle will be getting this building and testing in CI, otherwise it will go out of date quickly.

BUILD.bazel Outdated
)

go_binary(
name = "agent",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be able to name the resulting binary buildkite-agent with this!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh nice, that's an easy update

go_deps.from_file(go_mod = "//:go.mod")

# All *direct* dependencies are required to be listed explicitly
use_repo(go_deps, "com_github_aws_aws_sdk_go", "com_github_aws_aws_sdk_go_v2", "com_github_aws_aws_sdk_go_v2_config", "com_github_aws_aws_sdk_go_v2_feature_ec2_imds", "com_github_aws_aws_sdk_go_v2_service_kms", "com_github_azure_azure_sdk_for_go_sdk_azidentity", "com_github_azure_azure_sdk_for_go_sdk_storage_azblob", "com_github_brunoscheufler_aws_ecs_metadata_go", "com_github_buildkite_bintest_v3", "com_github_buildkite_go_pipeline", "com_github_buildkite_interpolate", "com_github_buildkite_roko", "com_github_buildkite_shellwords", "com_github_creack_pty", "com_github_datadog_datadog_go_v5", "com_github_denisbrodbeck_machineid", "com_github_dustin_go_humanize", "com_github_dustinkirkland_golang_petname", "com_github_gliderlabs_ssh", "com_github_go_chi_chi_v5", "com_github_gofrs_flock", "com_github_google_go_cmp", "com_github_google_go_querystring", "com_github_gowebpki_jcs", "com_github_khan_genqlient", "com_github_lestrrat_go_jwx_v2", "com_github_mattn_go_zglob", "com_github_oleiade_reflections", "com_github_opentracing_opentracing_go", "com_github_pborman_uuid", "com_github_puzpuzpuz_xsync_v2", "com_github_qri_io_jsonschema", "com_github_stretchr_testify", "com_github_urfave_cli", "com_google_cloud_go_compute_metadata", "dev_drjosh_zzglob", "in_gopkg_datadog_dd_trace_go_v1", "in_gopkg_yaml_v3", "io_opentelemetry_go_contrib_propagators_aws", "io_opentelemetry_go_contrib_propagators_b3", "io_opentelemetry_go_contrib_propagators_jaeger", "io_opentelemetry_go_contrib_propagators_ot", "io_opentelemetry_go_otel", "io_opentelemetry_go_otel_exporters_otlp_otlptrace", "io_opentelemetry_go_otel_exporters_otlp_otlptrace_otlptracegrpc", "io_opentelemetry_go_otel_sdk", "io_opentelemetry_go_otel_trace", "org_golang_google_api", "org_golang_x_crypto", "org_golang_x_exp", "org_golang_x_net", "org_golang_x_oauth2", "org_golang_x_sys", "org_golang_x_term", "tools_gotest_v3")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this generated? Seems like a thing that ought to be one per line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, very much generated! This was all via gazelle, so happy I didn't have to assemble by hand.

@CerealBoy
Copy link
Contributor Author

This is great! I haven't looked closely at what I assume gazelle generated, but seems fine.

The main hurdle will be getting this building and testing in CI, otherwise it will go out of date quickly.

There's 1 specific issue at the moment, and it's in the tests for the nested dependencies where I couldn't work out how to force the resolution. The last commits in the PR here give an indication on what I was trying and where the problem is.

Do you have any experiences that may be able to help out fixing this?

@CerealBoy CerealBoy force-pushed the test/adding-bazel branch 2 times, most recently from 4a0f26d to 2c97db7 Compare February 4, 2025 22:16
@CerealBoy
Copy link
Contributor Author

@DrJosh9000 If you had any ideas about what we could potentially do here, the output with error is as follows.

# bazelisk build //...
INFO: Analyzed 79 targets (0 packages loaded, 0 targets configured).
ERROR: /Users/ashone/buildkite/agent/internal/shell/BUILD.bazel:30:8: GoLink internal/shell/shell_test_/shell_test failed: (Exit 1): builder failed: error executing GoLink command (from target //internal/shell:shell_test) bazel-out/darwin_arm64-opt-exec-ST-d57f47055a04/bin/external/rules_go~~go_sdk~main___download_0/builder_reset/builder ... (remaining 13 arguments skipped)

Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
link: package conflict error: github.com/DataDog/datadog-agent/pkg/trace/stats: package imports github.com/DataDog/sketches-go/ddsketch/pb/sketchpb
          was compiled with: @@gazelle~~go_deps~com_github_datadog_sketches_go//ddsketch/pb/sketchpb:sketchpb
        but was linked with: @@gazelle~~go_deps~com_github_datadog_sketches_go//ddsketch/pb:sketchpb
See https://github.com/bazelbuild/rules_go/issues/1877.
Use --verbose_failures to see the command lines of failed build steps.
INFO: Elapsed time: 0.497s, Critical Path: 0.11s
INFO: 9 processes: 9 internal.
ERROR: Build did NOT complete successfully

@DrJosh9000
Copy link
Contributor

DrJosh9000 commented Feb 5, 2025

@DrJosh9000 If you had any ideas about what we could potentially do here, the output with error is as follows.

This is definitely a head-scratcher... and I don't have an answer.

I tried to bazel build //internal/shell:shell_test myself, and it gave me this debug hint (manually wrapped for readability):

DEBUG: /private/var/tmp/_bazel_josh/d44041637b0a500cde67752126364286/external/rules_go~/go/private/actions/link.bzl:59:18: 
Multiple copies of github.com/DataDog/sketches-go/ddsketch/pb/sketchpb passed to the linker.
Ignoring bazel-out/darwin_arm64-fastbuild/bin/external/gazelle~~go_deps~com_github_datadog_sketches_go/ddsketch/pb/sketchpb/sketchpb.a 
in favor of bazel-out/darwin_arm64-fastbuild/bin/external/gazelle~~go_deps~com_github_datadog_sketches_go/ddsketch/pb/sketchpb.a

which reads to me as though two different paths through the dependency graph are using two different definitions for sketchpb despite using build_file_proto_mode = "disable", and even though I read somewhere that even if it were operating in the default mode, gazelle should be ignoring .pb.go files.

So perhaps the resolve directives need to be spread around more. I did some digging with bazel cquery:

$ bazel cquery \
'allpaths(//internal/shell:shell_test,@@gazelle~~go_deps~com_github_datadog_sketches_go//ddsketch/pb/sketchpb:sketchpb)' \
  --output graph \
  | dot -Tsvg > ~/deps1.svg

deps1

$ bazel cquery \
'allpaths(//internal/shell:shell_test,@@gazelle~~go_deps~com_github_datadog_sketches_go//ddsketch/pb:sketchpb)' \
   --output graph \
   | dot -Tsvg > ~/deps2.svg

deps2

Unfortunately I still couldn't get it to work by adding resolve directives to the go_repositorys com_github_datadog_datadog_agent_pkg_trace and in_gopkg_datadog_dd_trace_go_v1.

Gazelle really insists on having two proto libraries!

Copy link
Contributor

@DrJosh9000 DrJosh9000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good news, @CerealBoy !

Comment on lines +7 to +9
go_deps = use_extension("@gazelle//:extensions.bzl", "go_deps")
go_deps.from_file(go_mod = "//:go.mod")

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've cracked it! It's ignoring all the overrides and fixes being attempted in deps.bzl because this is using BzlMod instead!

Here's how I think we can fix it:

  1. Get rid of deps.bzl, WORKSPACE.bazel, and the gazelle rule in BUILD.bazel
  2. Add a gazelle override here:
Suggested change
go_deps = use_extension("@gazelle//:extensions.bzl", "go_deps")
go_deps.from_file(go_mod = "//:go.mod")
go_deps = use_extension("@gazelle//:extensions.bzl", "go_deps")
go_deps.from_file(go_mod = "//:go.mod")
go_deps.gazelle_override(
directives = [
"gazelle:go_generate_proto false",
],
path = "github.com/DataDog/sketches-go",
)

With that I can successfully bazel build //... !

@CerealBoy CerealBoy marked this pull request as ready for review February 6, 2025 04:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants