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 grpc version #7535

Closed
wants to merge 2 commits into from
Closed

update grpc version #7535

wants to merge 2 commits into from

Conversation

aranguyen
Copy link

update grpc to the latest version to enable the flag flip for --incompatible_use_platforms_repo_for_constraints bazelbuild/continuous-integration#1404

@aranguyen aranguyen marked this pull request as ready for review September 16, 2022 18:09
@dbaileychess
Copy link
Collaborator

The .blazerc shouldn't be checked in.

@aranguyen
Copy link
Author

aranguyen commented Sep 19, 2022

@dbaileychess without the .bazelrc file and the flag, the build will fail upgrading to grpc 1.49.0. I could also add the flag to presubmit.yml file but that will only work for CI and regular users would see issue when building flatbuffers. Do you have any other suggestion to get around this issue? and why can't we check in .bazelrc file?

@meteorcloudy for visibility + additional advise.

It's worth noting that this is blocking the --incompatible_use_platforms_repo_for_constraint flag flip which blocks our Bazel 6.0 release.

@meteorcloudy
Copy link
Contributor

I think the context is: grpc/grpc#29750

@dbaileychess
Copy link
Collaborator

@aranguyen RC files are meant for local configuration overrides and shouldn't be the default way to enforce common configuration changes.

@aranguyen
Copy link
Author

@dbaileychess this is the workspace level .bazelrc file which is read by Bazel by default unless --noworkspace is specified so I think it is ok to use it for this use case. Are you talking about the user-defined rc filed which is normally used for local overrides and not read by Bazel by default?

If this still doesn't work, do you have any other suggestion?

@aranguyen
Copy link
Author

@dbaileychess I looked around a bit more to see if there is a better alternative. But having this flag in .bazelrc file seems to be the correct approach. Please see discussion https://stackoverflow.com/questions/40260242/how-to-set-c-standard-version-when-build-with-bazel

aranguyen added a commit to aranguyen/continuous-integration that referenced this pull request Sep 20, 2022
@dbaileychess
Copy link
Collaborator

@aranguyen I am just worried because that means everyone who builds flatbuffers (with or without grpc) is forced to go to C++14. If its just CI failing, we should use the command-line way to specify the flag. For users who are using grpc, when they get the grpc repo, it should come with that tools/blaze.rc file.

Correct me if I'm wrong.

@meteorcloudy
Copy link
Contributor

I see, flatbuffer only needs grpc for testing: https://github.com/google/flatbuffers/search?q=com_github_grpc_grpc

Then indeed it doesn't make sense to force flatbuffer to be built with C++14.

The problem here is because grpc added this flag in their bazelrc file, but bazelrc doesn't propagate downstream, so this flag is missing when building grpc as a dependency, and it's broken in grpc 1.49.0. I have suggested to move this flag to some bzl file that defines their common c++ options. grpc/grpc#29750 (comment)

In the meantime, we can patch grpc 1.49.0 to make it work. Sent #7538

@aranguyen
Copy link
Author

thank you both for the discussion. Since #7538 is sent out, this pr is obsolete...closing.

@aranguyen aranguyen closed this Sep 21, 2022
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.

None yet

3 participants