Skip to content

Build Target Reorganization Part 1#30518

Merged
ralphchung merged 10 commits intogrpc:masterfrom
ralphchung:build-target-reorganization
Aug 11, 2022
Merged

Build Target Reorganization Part 1#30518
ralphchung merged 10 commits intogrpc:masterfrom
ralphchung:build-target-reorganization

Conversation

@ralphchung
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 force-pushed the build-target-reorganization branch from 6fe0eb2 to 653c295 Compare August 8, 2022 15:15
@ralphchung ralphchung force-pushed the build-target-reorganization branch from a43239a to 6a6b0c5 Compare August 11, 2022 17:21
@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.

3 participants