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

Update dependencies, buildbarn ecosystem to latest #29

Merged
merged 17 commits into from
Sep 10, 2023

Conversation

minor-fixes
Copy link
Collaborator

@minor-fixes minor-fixes commented Apr 6, 2023

This change brings bb-remote-asset up-to-date with bb-storage/bb-remote-execution, so that nominally they may be able to use compatible configs, and likely inherit the other benefits (more recent toolchain, better bazel integration, etc.)

This is done largely by copying WORKSPACE from bb-storage, making sure to match versions and orderings to ensure that the same versions of dependencies are loaded.

Other transforms/migrations that proved necessary:

  • HTTPClient was eliminated from bb-storage; mocking had to be done on a http.RoundTripper instead, in the same manner as buildbarn/bb-storage@b9951ac
  • Addition of structured concurrency to main, similar to buildbarn/bb-storage@4f243ea
  • Addition of http.ClientConfiguration proto options to HttpFetcherConfiguration
  • bazel.canonical_id added to the list of supported qualifiers, which prevents errors in some (most?) WORKSPACE configurations

Tested:

TODO:

@minor-fixes
Copy link
Collaborator Author

Currently running into some compile issues, and could use a little guidance:

ERROR: /home/scott/dev/third_party/bb-remote-asset/pkg/storage/BUILD.bazel:3:11: GoCompilePkg pkg/storage/storage.a failed: (Exit 1): builder failed: error executing command (from target //pkg/storage:storage) bazel-out/k8-opt-exec-2B5CBBC6-ST-a2b97ed4b8d6/bin/external/go_sdk/builder_reset/builder compilepkg -sdk external/go_sdk -installsuffix linux_amd64 -src pkg/storage/action_cache_asset_store.go -src ... (remaining 55 arguments skipped)

Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
pkg/storage/action_cache_asset_store.go:37:26: instance.NewDigestFromProto undefined (type "github.com/buildbarn/bb-storage/pkg/digest".InstanceName has no field or method NewDigestFromProto)
pkg/storage/action_cache_asset_store.go:58:31: instance.NewDigestFromProto undefined (type "github.com/buildbarn/bb-storage/pkg/digest".InstanceName has no field or method NewDigestFromProto)
pkg/storage/action_cache_asset_store.go:130:26: instance.NewDigestFromProto undefined (type "github.com/buildbarn/bb-storage/pkg/digest".InstanceName has no field or method NewDigestFromProto)
pkg/storage/action_cache_asset_store.go:182:37: instance.NewDigestFromProto undefined (type "github.com/buildbarn/bb-storage/pkg/digest".InstanceName has no field or method NewDigestFromProto)
pkg/storage/action_cache_asset_store.go:217:34: instance.NewDigestFromProto undefined (type "github.com/buildbarn/bb-storage/pkg/digest".InstanceName has no field or method NewDigestFromProto)
pkg/storage/action_cache_asset_store.go:234:35: instance.NewDigestFromProto undefined (type "github.com/buildbarn/bb-storage/pkg/digest".InstanceName has no field or method NewDigestFromProto)
pkg/storage/action_cache_asset_store.go:266:33: instance.NewDigestFromProto undefined (type "github.com/buildbarn/bb-storage/pkg/digest".InstanceName has no field or method NewDigestFromProto)
pkg/storage/action_cache_asset_store.go:308:26: instance.NewDigestFromProto undefined (type "github.com/buildbarn/bb-storage/pkg/digest".InstanceName has no field or method NewDigestFromProto)
pkg/storage/asset_reference.go:38:18: instance.NewDigest undefined (type "github.com/buildbarn/bb-storage/pkg/digest".InstanceName has no field or method NewDigest)

This appears to be due to the modifications in bb-storage around support of SHA256TREE, which cause the ontology of instances/digests to shift a bit, from inspection of buildbarn/bb-storage@2d0d7ee. @EdSchouten maybe you can give me some guidance on what an acceptable way to get this working at least (even if not supporting say SHA256TREE right away) Some things I've been wondering:

  • It seems that most protobuf requests in REAPI that take a Digest now also take a DigestFunction.Value to inform as to the function used to compute Digest, from https://github.com/bazelbuild/remote-apis/pull/235/files - am I correct in presuming that the Remote Asset API will need to be updated to pass functions for digests in requests and responses as well?
  • Why not put the digest function inside the Digest message itself, if they go together? (maybe some compatibility issue prevents this from being feasible?)
  • Would above proto changes be necessary for remote-asset-api, or is it possible to plumb digest function info from somewhere else, dropping support for SHA256TREE here (at least temporarily, if proto changes need to land first)

I'm a bit new to the buildbarn codebase, so any pointers here would be greatly appreciated!

WORKSPACE Outdated
Comment on lines 134 to 136
strip_prefix = "googleapis-143084a2624b6591ee1f9d23e7f5241856642f4d",
urls = ["https://github.com/googleapis/googleapis/archive/143084a2624b6591ee1f9d23e7f5241856642f4d.zip"],
)
Copy link
Collaborator Author

@minor-fixes minor-fixes Apr 6, 2023

Choose a reason for hiding this comment

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

@googleapis is extra over bb-storage's WORKSPACE; AFAICT this is because @googleapis is required for remote-apis but bb-storage patches remote-apis to use @go_googleapis instead sufficiently for its usecase (but not sufficiently for the remote asset proto).

Either the patch to remote-apis needs to be reworked to cover more of the protos, or maybe remote-apis can be fixed in a way that doesn't necessitate the patch, but having this here doesn't seem to be causing build issues yet.

@minor-fixes minor-fixes marked this pull request as ready for review April 6, 2023 15:11
@EdSchouten
Copy link
Member

Yes, that would be ideal. This was also being discussed in the PR itself:

bazelbuild/remote-apis#235 (comment)

What you can do for the time being is that you use a construct like this:

https://github.com/buildbarn/bb-storage/blob/master/pkg/blobstore/grpcservers/content_addressable_storage_server.go#L31-L41

But with s/in.DigestFunction/remoteexecution.DigestFunction_UNKNOWN/. This will cause the digest library to behave the same way as before: not use an explicit digest function field, but infer it from the length of the digest provided. This obviously has the disadvantage that SHA256TREE will not be supported.

  • Why not put the digest function inside the Digest message itself, if they go together? (maybe some compatibility issue prevents this from being feasible?)

One of the things we also wanted to rectify in REv2 was that the protocol would allow you to construct input roots with heterogeneous digest functions. So a SHA-256 based action with an MD5 input root with a SHA-1 subdirectory. This isn't desirable, as it lacks normalization. This is why we made it so that the digest function needs to be provided on a per request basis. Not a per Digest basis.

  • Would above proto changes be necessary for remote-asset-api, or is it possible to plumb digest function info from somewhere else, dropping support for SHA256TREE here (at least temporarily, if proto changes need to land first)

I'm a bit new to the buildbarn codebase, so any pointers here would be greatly appreciated!

No worries. Doing a great job!

minor-fixes and others added 3 commits April 10, 2023 11:29
Everything builds (so far) except using an outdated protoc-gen-go for
some reason
`bazel test //...` works! (with modified patch on bb-storage)
Some cleanup
@minor-fixes minor-fixes requested a review from EdSchouten May 10, 2023 04:22
@minor-fixes
Copy link
Collaborator Author

Updated the github workflows and did some preliminary local testing via gh act - can you give it another kick?

This commit grabs the latest bb-storage and bb-remote-execution, and
their transitive updates to third-party repos.
pkg/fetch/http_fetcher_test.go Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
pkg/storage/action_cache_asset_store.go Outdated Show resolved Hide resolved
@minor-fixes minor-fixes merged commit 52b25db into buildbarn:master Sep 10, 2023
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.

3 participants