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

Build Target Reorganization Part 1 #30518

Merged
merged 10 commits into from
Aug 11, 2022

Conversation

ralphchung
Copy link
Contributor

@ralphchung ralphchung commented Aug 5, 2022

Given that we are removing codegen related files, we need to clean up the build targets in the current build file. There will be a series of PRs for this.

The new build targets should roughly follow this dependency graph.

┌───────┐┌───────────────┐        
│grpc++ ││grpc++_unsecure│        
└┬─────┬┘└┬─────────┬────┘        
┌▽───┐┌▽──▽───────┐┌▽────────────┐
│grpc││grpc++_base││grpc_unsecure│
└┬───┘└┬──────────┘└┬────────────┘
┌▽─────▽────────────▽┐            
│grpc_base           │            
└────────────────────┘            

Detailed steps is the following.

  • (Covered in this PR) gpr_base is merged into gpr. The gpr here itself is nothing but depending on gpr_base. I don't think we need to separate them.
  • grpc++_internals and grpc++_internal_hdrs_only are merged into grpc++. The reason is similar. grpc++ does not contain much stuff but grpc++_internals.
  • (Covered in this PR) gpr_codegen is merged to gpr since the files in gpr_codegen should be removed in the future and be moved to gpr.
  • grpc_codegen is merged to grpc_base with the similar reason as above.
  • grpc++_codegen_base and grpc++_codegen_base_src are merged to grpc++_base with the similar reason as above.
  • grpc_secure is merged to grpc since grpc is by default secure. We do not need to separate them.
  • grpc++_base_unsecure is merged either into grpc++_base or into grpc++_unsecure.

This PR removes gpr_base and merges gpr_codegen into gpr without removing gpr_codegen for temporary compatibility. It should be removed in the following PRs.

BUILD Outdated
tags = ["nofixdeps"],
visibility = ["@grpc:alt_gpr_base_legacy"],
tags = [
"avoid_dep",
Copy link
Member

Choose a reason for hiding this comment

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

Suspect we should remove the avoid_dep here... could you try and see whether fix_build_deps stays rational?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried, and I did not find anything change because of this tag.

Copy link
Member

@ctiller ctiller left a comment

Choose a reason for hiding this comment

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

Please do a trial import and see if there's any problems there before submitting.

Copy link
Contributor

@Vignesh2208 Vignesh2208 left a comment

Choose a reason for hiding this comment

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

If the goal is to eliminate gpr_base entirely, that will most definitely require a cherrypick since gpr_base is used in some places internally as well. Please confirm! Thanks!

@ralphchung ralphchung merged commit 543b290 into grpc:master Aug 11, 2022
@ralphchung ralphchung deleted the build-target-reorganization branch August 11, 2022 20:58
@copybara-service copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label Aug 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bloat/none imported Specifies if the PR has been imported to the internal repository lang/core per-call-memory/neutral per-channel-memory/neutral release notes: no Indicates if PR should not be in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants