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

Artifacts such as grpc-googleapis, grpc-xds missing from IO_GRPC_GRPC_JAVA_OVERRIDE_TARGETS #9162

Closed
SanjayVas opened this issue May 11, 2022 · 3 comments · Fixed by #9172
Closed
Milestone

Comments

@SanjayVas
Copy link

What version of gRPC-Java are you using?

1.46.0

What is your environment?

Linux (gLinux, based on Debian Bookworm)

What did you expect to see?

All Maven artifacts that are build from this repo are overridden when using IO_GRPC_GRPC_JAVA_OVERRIDE_TARGETS in repositoriez.bzl, such that they are built from source and not pulled from Maven.

What did you see instead?

Some artifacts such as grpc-googleapis and grpc-xds are pulled from Maven instead of being built from grpc-java source.

Steps to reproduce the bug

  1. Depend on grpc-java as a Bazel repo target.
  2. Specify any artifact that depends on io.grpc:grpc-googleapis in maven_install.
  3. Build.

Notes

When depending on grpc-java as a Bazel repo, one is building grpc-java from source. This means that the Bazel project should not pull any Maven artifacts that come from grpc-java, as you could end up with multiple copies/versions of a given library.

@ejona86
Copy link
Member

ejona86 commented May 13, 2022

Well, currently googleapis and xds aren't built with Bazel at all. The Bazel build was a community contribution and we've kept it running and modernized it as time has gone on (including migrating to maven_install). But it has never been fully comprehensive. The community has mostly added the parts they need. xds will probably be reasonably annoying to produce a build, since it has many dependencies.

This means that the Bazel project should not pull any Maven artifacts that come from grpc-java, as you could end up with multiple copies/versions of a given library.

What libraries are you seeing duplicated? Looking through the BUILD.bazel that we have and comparing to IO_GRPC_GRPC_JAVA_OVERRIDE_TARGETS, it seems grpc-rls is main one missing and it is unlikely that anyone is using its Bazel targets. grpc-services is also missing, but I think the bazel build targets there are incomplete.

@SanjayVas
Copy link
Author

The main concern when depending on grpc-java via Bazel is making sure there aren't compatibility issues. Things can get messy with common dependencies if some of the libraries from the same codebase are built from source and others pulled precompiled from Maven.

It's theoretically fine if two conditions are true:

  1. The Maven artifacts were built from the same source version as what's being built via Bazel. This can be accomplished by always using a released version of the source repo (as opposed to having the freedom to use an arbitrary revision) and ensuring that you specify that version for all of the relevant artifacts.
  2. The libraries are structured in such a way that the ones being built by Bazel can be mapped to Maven coordinates.

I believe that (2) is generally true for the libraries that can be built from Bazel, in which case there isn't actually a duplication concern.

My expectation was that if grpc-java could be built using Bazel, this meant that all libraries in the grpc-java repo could be built using Bazel. It appears that this is neither true nor is there an intent to make it so. Regardless, if we follow (1) then we can still have correct builds.

If there's at least a list of Maven coordinates maintained in grpc-java that can be consumed by Bazel, then it should be easier to maintain (1). We had been assuming that IO_GRPC_GRPC_JAVA_OVERRIDE_TARGETS would be that list. Right now our code to do that looks like the snippet below. What we're seeking for maintainability is to clean up that TODO.

def _grpc_java_override_artifact_dict():
    """Returns a dict of coordinates to version for grpc-java overrides."""
    artifacts_dict = {}

    for coordinates, target in IO_GRPC_GRPC_JAVA_OVERRIDE_TARGETS.items():
        if target.startswith("@io_grpc_grpc_java//"):
            artifacts_dict[coordinates] = GRPC_JAVA_VERSION
        elif target.startswith("@@com_google_protobuf//"):
            artifacts_dict[coordinates] = PROTOBUF_VERSION

    # TODO(grpc/grpc-java#9162): Drop once all the right artifacts are included
    # in IO_GRPC_GRPC_JAVA_OVERRIDE_TARGETS.
    artifacts_dict.update({
        "io.grpc:grpc-googleapis": GRPC_JAVA_VERSION,
        "io.grpc:grpc-services": GRPC_JAVA_VERSION,
        "io.grpc:grpc-xds": GRPC_JAVA_VERSION,
    })

    return artifacts_dict

ejona86 added a commit to ejona86/grpc-java that referenced this issue May 14, 2022
Not updating the example WORKSPACE because it doesn't have any
Bazel-enabled build that depends on xds and so doesn't need the
additional repository dependencies.

Fixes grpc#9162
ejona86 added a commit to ejona86/grpc-java that referenced this issue May 14, 2022
Not updating the example WORKSPACE because it doesn't have any
Bazel-enabled build that depends on xds and so doesn't need the
additional repository dependencies.

Fixes grpc#9162
ejona86 added a commit to ejona86/grpc-java that referenced this issue May 14, 2022
Not updating the example WORKSPACE because it doesn't have any
Bazel-enabled build that depends on xds and so doesn't need the
additional repository dependencies.

Fixes grpc#9162
@ejona86
Copy link
Member

ejona86 commented May 14, 2022

We had been assuming that IO_GRPC_GRPC_JAVA_OVERRIDE_TARGETS would be that list.

That doesn't really help too much, as Bazel will resolve effective versions and so deps could be upgraded. It warns about duplicates in the list, but I don't think it warns about general upgrading of listed deps. I've added a BUILD for xds and some others in #9172. Xds was indeed annoying.

ejona86 added a commit that referenced this issue May 16, 2022
Not updating the example WORKSPACE because it doesn't have any
Bazel-enabled build that depends on xds and so doesn't need the
additional repository dependencies.

Fixes #9162
@ejona86 ejona86 added this to the v1.47 milestone May 16, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants