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 Improvements #9855

Merged
merged 3 commits into from Feb 6, 2023
Merged

Build Improvements #9855

merged 3 commits into from Feb 6, 2023

Conversation

Faqa
Copy link
Contributor

@Faqa Faqa commented Jan 23, 2023

This PR fixes a pair of issues that prevent complete cacheability of certain tasks in the project's build:

  1. All proto tasks rely on the root build.gradle file in such a way that the dependency is registered with the full absolute path of the project on the local node. This makes it impossible to cache those tasks between two nodes with a different directory layout. Fixed by specifying that the file path should be relative to the project dir.
  2. The NettyResourceTransformer object in netty/shaded/build.gradle is not explicitly marked as cacheable. This, I admit, I took more of a guess that it was an oversight, but if there's a reason, I can drop that from the PR.

UPDATE:

These are the build scans that demonstrate the change:

  1. Build scan from master
  2. Build scan from PR

You can clearly see here that tasks move "not cacheable" to "cacheable" as a result of this change.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jan 23, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link
Contributor

@runningcode runningcode left a comment

Choose a reason for hiding this comment

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

looks good. just one suggestion. we should give the property a name so that it is easier to see and track.

build.gradle Outdated Show resolved Hide resolved
Co-authored-by: Nelson Osacky <nelson@osacky.com>
@larry-safran larry-safran added the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Feb 6, 2023
@grpc-kokoro grpc-kokoro removed the kokoro:force-run Add this label to a PR to tell Kokoro to re-run all tests. Not generally necessary label Feb 6, 2023
Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

Yeah, we don't care about cachability in our build at all. I don't trust the feature enough to use it for the CI, which is where it'd do us the most good.

@larry-safran larry-safran merged commit 56a08c3 into grpc:master Feb 6, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 8, 2023
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.

None yet

5 participants