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

[Deps] Adding upb as a submodule #34199

Merged
merged 3 commits into from
Aug 29, 2023
Merged

[Deps] Adding upb as a submodule #34199

merged 3 commits into from
Aug 29, 2023

Conversation

veblush
Copy link
Contributor

@veblush veblush commented Aug 29, 2023

Let's make it as a regular dependency like others. So gRPC is now unvendoring it first. Project generation part will be followed later.

@veblush veblush added the release notes: yes Indicates if PR needs to be in release notes label Aug 29, 2023
@veblush veblush changed the title Adding upb as a submodule [Deps] Adding upb as a submodule Aug 29, 2023
@veblush
Copy link
Contributor Author

veblush commented Aug 29, 2023

Failing tests are caused by changing subtree to submodule. This won't happen once this is merged.

@veblush veblush enabled auto-merge (squash) August 29, 2023 19:57
@veblush veblush merged commit f6a9942 into grpc:master Aug 29, 2023
59 of 69 checks passed
@jtattermusch
Copy link
Contributor

Hm, IMHO changing upb into a submodule isn't really possible without also switching our cmake build to using the upb's CMakeLists.txt (since for anything that isn't vendored in our repo directly, we need to depend on a properly installable library).

Looks like this PR is breaking the C++ distribtests precisely for this reason:
https://source.cloud.google.com/results/invocations/1cde23c8-bba2-4e69-9b63-0baac91d8a77/targets
https://source.cloud.google.com/results/invocations/12b587f0-ff07-4712-9136-ab82eb8d0259/targets/github%2Fgrpc/tests

So more followup work is needed.

@copybara-service copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label Aug 30, 2023
@matthewjmiller1
Copy link
Contributor

Hm, IMHO changing upb into a submodule isn't really possible without also switching our cmake build to using the upb's CMakeLists.txt (since for anything that isn't vendored in our repo directly, we need to depend on a properly installable library).

Looks like this PR is breaking the C++ distribtests precisely for this reason: https://source.cloud.google.com/results/invocations/1cde23c8-bba2-4e69-9b63-0baac91d8a77/targets https://source.cloud.google.com/results/invocations/12b587f0-ff07-4712-9136-ab82eb8d0259/targets/github%2Fgrpc/tests

So more followup work is needed.

Is there anything tracking this followup work?

As far as I understand it, this commit violated this invariant:

https://github.com/grpc/grpc/blob/master/third_party/README.md

gRPC C++ needs to stay buildable/installable even if the submodules are not present (e.g. the tar.gz archive with gRPC doesn't contain the submodules), assuming that the dependencies are already installed. This is a requirement for being able to provide a reasonable install process (e.g. using cmake) and to support package managers for gRPC C++.

Since we can no longer build/install gRPC if the upb submodule is not present, even if it is already installed.

@matthewjmiller1
Copy link
Contributor

It also looks like upb itself may not support building via cmake:
https://github.com/protocolbuffers/protobuf/blob/main/upb/cmake/README.md
Which, even if gRPC were to support using upb as an external dependency instead of a submodule, still would make gRPC unusable in environments where all the dependencies are built from source using cmake.

veblush added a commit that referenced this pull request Sep 27, 2023
Temporary gRPC is vendoing upb again until upb has a cmake support.
(Rollback of #34199)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/build imported Specifies if the PR has been imported to the internal repository lang/core release notes: yes Indicates if PR needs to be in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants