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

Include com_google_protobuf_javalite to MODULE.bazel to fix bzlmod querying graph in end-user repo #11147

Merged
merged 6 commits into from
May 15, 2024

Conversation

firov
Copy link
Contributor

@firov firov commented May 3, 2024

Query in end-user repository fails with:

bazel query --notool_deps --noimplicit_deps "deps(//...)" --output graph

ERROR: Evaluation of query failed: preloading transitive closure failed: no such package '@@[unknown repo 'com_google_protobuf_javalite' requested from @https://github.com/grpc-java~]//': The repository '@@[unknown repo 'com_google_protobuf_javalite' requested from @https://github.com/grpc-java~]' could not be resolved: No repository visible as '@com_google_protobuf_javalite' from repository '@https://github.com/grpc-java~'

It's because com_google_protobuf_javalite is missing in use_repo in MODULE.bazel and completely not imported in repositories.bzl when using bzlmod. I noticed that actually com_google_protobuf_javalite is an archive with protobuf repository, which we already have, so i just change remapping.

@firov firov changed the title Fix 3d party dependency use_repo Use java_lite from protobuf module to fix graph query problems May 3, 2024
@ejona86
Copy link
Member

ejona86 commented May 6, 2024

com_google_protobuf_javalite was load-bearing in previous versions of Bazel, as java_lite_proto_library used it. I can totally believe it isn't needed in newer versions, but we'd need to make sure it isn't needed by versions of Bazel we still support.

The easier approach is to just define com_google_protobuf_javalite as a duplicate of com_google_protobuf_java, which is what we've been doing for a while. (In the olden days, protobuf had a separate branch for lite, but it was later integrated into the master branch.)

@firov
Copy link
Contributor Author

firov commented May 6, 2024

The easier approach is to just define com_google_protobuf_javalite as a duplicate of com_google_protobuf_java, which is what we've been doing for a while.

It won't work in bzlmod because of module strict visibility, it must be accessible from grpc-java itself. So currently we just patch it on module import.

but we'd need to make sure it isn't needed by versions of Bazel we still support.

I don't remove it completely, I thought this remapping will do the trick
https://github.com/grpc/grpc-java/pull/11147/files#diff-c8bacb964b6cf475b85b8fa254a2769dcf34f28b813437048f877aa3ffa3d71bL65-R65
But i can imaging situation in workspace variant when you reference it directly and don't import by yourself. But I think it's ok to just replace in end user code when referencing to protobuf as it is the same library.

Currently in bzlmod perspective trunk is not correct as it has a reference to repository com_google_protobuf_javalite and does not define repository with that name. So other possible fix is remove if not bzlmod and not for com_google_protobuf_javalite and add it to use_repo macros in MODULE.bazel.

@ejona86
Copy link
Member

ejona86 commented May 6, 2024

It won't work in bzlmod because of module strict visibility, it must be accessible from grpc-java itself.

I was meaning to change grpc-java to add the repo. (So changing not bzlmod and add an add_repo sounds fine.)

I don't remove it completely, I thought this remapping will do the trick

The problem is we need to make sure to use the same library as java_lite_proto_library. So it is really a question of java_lite_proto_library's behavior and then we match it (with tweaks for migration when it changes).

@firov firov changed the title Use java_lite from protobuf module to fix graph query problems Include com_google_protobuf_javalite to MODULE.bazel to fix bzlmod querying graph in end-user repo May 7, 2024
@firov
Copy link
Contributor Author

firov commented May 7, 2024

I was meaning to change grpc-java to add the repo. (So changing not bzlmod and add an add_repo sounds fine.)

Okay, let's go this way.

The problem is we need to make sure to use the same library as java_lite_proto_library. So it is really a question of java_lite_proto_library's behavior and then we match it (with tweaks for migration when it changes).

Ah i got it now, if user redefines it in workspace, we need to use his version.

@ejona86 ejona86 requested a review from kannanjgithub May 7, 2024 14:46
@ejona86 ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label May 7, 2024
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label May 7, 2024
@ar3s3ru
Copy link

ar3s3ru commented May 15, 2024

Hi folks, just hit this issue. Can this be merged, or is there a different way in which it can be fixed on my end?

@firov
Copy link
Contributor Author

firov commented May 15, 2024

Hi folks, just hit this issue. Can this be merged, or is there a different way in which it can be fixed on my end?

Hey, as i see we are waiting for second review. For now you can import this pr as a patch, if you are using version from central registry, something like this:

archive_override(
    module_name = "grpc-java",
    integrity = "sha256-MM/JVMIXRJOCJgGnS4doN5hsRE2bnpFwgplLXjQ0jzQ=",
    patch_strip = 1,
    patches = [
        "//:patches/grpc-java/add_module_bazel.patch",
        "//:patches/grpc-java/maven_lockfile.patch",
        "//:patches/grpc-java/update_labels.patch",
        "//:patches/grpc-java/javalite.patch"
    ],
    strip_prefix = "grpc-java-1.62.2",
    urls = [
        "https://github.com/grpc/grpc-java/archive/refs/tags/v1.62.2.tar.gz",
    ],
)

where first 3 patches you can take from registry https://github.com/bazelbuild/bazel-central-registry/tree/main/modules/grpc-java/1.62.2/patches
And 4th is from pr

@kannanjgithub kannanjgithub merged commit f995c12 into grpc:master May 15, 2024
13 checks passed
@firov firov deleted the bazel_use_repo_fix branch May 15, 2024 11:51
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants