Fix C++ cleanup in Grpc.Tools by Falco20019 · Pull Request #22895 · grpc/grpc

Falco20019 · 2020-05-07T17:43:37Z

The result of Protobuf_ExpectedOutputs in Protobuf_PrepareClean and Protobuf_PrepareCompile is different for out-of-path protos if no ProtoRoot was given.

This is, because _Protobuf_SelectFiles is setting ProtoRoot=%(RelativeDir) when building, but this is not set when cleaning up.

This PR extracts the logic of settings the ProtoRoot from _Protobuf_SelectFiles into _Protobuf_SetProtoRoot and ensures, that the same files are expected for creation and cleanup. Since we are not filling a new item group but updating an existing one, we had to use update logic instead of include logic. Otherwise we would end up with Protobuf having the same item twice, once without ProtoRoot and once with the set ProtoRoot.

CC @jtattermusch @kkm000

jtattermusch

I have trouble following the logic changes and making sure this doesn't break anything.
We really need to come up with some simple way of testing the .targets file, the lack of tests is making accepting any Grpc.Tools change difficult.

src/csharp/Grpc.Tools/build/_protobuf/Google.Protobuf.Tools.targets

Falco20019 · 2020-05-25T06:32:44Z

@jtattermusch @JunTaoLuo I updated the PR to use Protobuf_Rooted as intermediate item group and only split up the original logic from Protobuf_Compile into Protobuf_Rooted and Protobuf_Compile. This increases the data overhead as a new item group is necessary. But at least it's easier to follow and fixes the bug without introducing new logic.

src/csharp/Grpc.Tools/build/_protobuf/Google.Protobuf.Tools.targets

JunTaoLuo · 2020-05-26T21:07:20Z

src/csharp/Grpc.Tools/build/_protobuf/Google.Protobuf.Tools.targets


       All ExpectedOutputs will be removed. -->
-  <Target Name="Protobuf_PrepareClean" Condition=" '@(Protobuf)' != '' ">
+  <Target Name="Protobuf_PrepareClean" Condition=" '@(Protobuf_Rooted)' != '' ">


Same here, I think it would make sense for Protobuf_PrepareClean to depend on _Protobuf_SetProtoRoot, especially since this target isn't prefixed with _ which generally signals that it could be called independently.

Good point! I will change it.

Are you still going to address this?

See #22895 (comment)

We have two options how to fix it but both have downsides and would change either the current behavior or would by asymmetrical to how creation is currently done which feels a bit strange. Would be good to @JunTaoLuo‘s opinion on the two suggestions. (Or three, since keeping it this way is also one.)

Let's leave as is for now and if needed, we can address in a followup PR.

JunTaoLuo

Changes look reasonable to me.

Falco20019 · 2020-05-27T07:30:52Z

@JunTaoLuo I looked into having Protobuf_PrepareClean depend on _Protobuf_SetProtoRoot. It seems that Condition is checked before DependsOnTargets. So adding it to Protobuf_PrepareClean currently didn't work as it will skip it.

I see two options here on how to solve it:

Split Protobuf_PrepareClean to use a sub-task:

  <Target Name="Protobuf_PrepareClean"
          Condition=" '@(Protobuf)' != '' "
          DependsOnTargets=" _Protobuf_SetProtoRoot;
                             _Protobuf_PrepareClean" />
							 
  <Target Name="_Protobuf_PrepareClean" Condition=" '@(Protobuf_Rooted)' != '' ">
    <!-- Predict expected names. -->
    <ProtoCompilerOutputs Condition=" '$(Language)' == 'C#' "
                          Protobuf="@(Protobuf_Rooted)"
                          Generator="$(Protobuf_Generator)">
      <Output TaskParameter="PossibleOutputs" ItemName="Protobuf_ExpectedOutputs" />
    </ProtoCompilerOutputs>
  </Target>

Use @(Protobuf) as Condition as this is the real condition for the whole target.

  <Target Name="Protobuf_PrepareClean"
          DependsOnTargets=" _Protobuf_SetProtoRoot"
          Condition=" '@(Protobuf)' != '' ">
    <!-- Predict expected names. -->
    <ProtoCompilerOutputs Condition=" '$(Language)' == 'C#' "
                          Protobuf="@(Protobuf_Rooted)"
                          Generator="$(Protobuf_Generator)">
      <Output TaskParameter="PossibleOutputs" ItemName="Protobuf_ExpectedOutputs" />
    </ProtoCompilerOutputs>
  </Target>

We have the same issue with Protobuf_PrepareCompile and Protobuf_Compile. Protobuf_PrepareCompile is expecting @(Protobuf_Compile) to be set, which is only done through _Protobuf_SelectFiles relying on _Protobuf_SetProtoRoot. So we might want to also apply whatever fix we use for Protobuf_PrepareClean there too.

Both ways would change the current behavior. So if someone is calling the target already with manually setting @(Protobuf_Compile) for Protobuf_PrepareCompile, this would be a breaking change. So maybe keeping _Protobuf_SetProtoRoot in Protobuf_Compile and Protobuf_Clean would be less intrusive.

Falco20019 · 2020-06-04T06:17:20Z

@JunTaoLuo @jtattermusch Any more input on this? Thanks for the valuable feedback so far!

jtattermusch · 2020-06-04T12:21:12Z

Adhoc distribtest run: sponge/ec2617f6-ed25-4bca-a3b6-cc1ddcc1df08

jtattermusch · 2020-06-04T12:32:12Z

@Falco20019 can you update the PR description to match the reality? (right now it seems out of data). Also, why is this called "fix C++ cleanup"? - there doesn't seem anything that's specific to C++ codegen in this change.

jtattermusch

I think with some very minor nits (see my new comments), this is looking good now.
Thanks for improving the clarity of the change, following what it does is now much easier.

I'll approve once the distribtests pass and my last comments are addressed.

Falco20019 · 2020-06-04T20:05:03Z

@Falco20019 can you update the PR description to match the reality? (right now it seems out of data). Also, why is this called "fix C++ cleanup"? - there doesn't seem anything that's specific to C++ codegen in this change.

It fixes the root that was missing for C++ cleanup. Right now, since the root was not set, the expected files are wrong and cleanup is not working for C++. So this really only is a fix for C++ as that’s the only language using the proto root in the expected output. I want to use the same logic also for C#. To be more precise, currently for creation the root is set through ProtoCompile, but for cleanup it was not, so the result was different for creation and cleanup.

jtattermusch · 2020-06-05T06:26:42Z

Another adhoc run (this time with rebased code to avoid an unrelated artifact breakage).
https://sponge.corp.google.com/invocation?id=57315f8b-7c14-413e-9443-c3a0d50a99ad&searchFor=

jtattermusch · 2020-06-05T12:53:11Z

The distribtests have passed: sponge/580024b2-08f1-472e-bc7a-3ba0f3aba111

jtattermusch

LGTM.

Fix CPP cleanup

eb8c618

Falco20019 mentioned this pull request May 7, 2020

Avoid collisions in cs files generated by Grpc.Tools #22869

Merged

jtattermusch self-assigned this May 8, 2020

jtattermusch self-requested a review May 8, 2020 13:51

jtattermusch added lang/C# release notes: yes Indicates if PR needs to be in release notes kokoro:run labels May 8, 2020

grpc-kokoro removed the kokoro:run label May 8, 2020

jtattermusch reviewed May 15, 2020

View reviewed changes

src/csharp/Grpc.Tools/build/_protobuf/Google.Protobuf.Tools.targets Outdated Show resolved Hide resolved

JunTaoLuo reviewed May 19, 2020

View reviewed changes

src/csharp/Grpc.Tools/build/_protobuf/Google.Protobuf.Tools.targets Outdated Show resolved Hide resolved

Use intermediate group Protobuf_Rooted

aae5e4c

Falco20019 requested a review from jtattermusch May 25, 2020 06:29

JunTaoLuo reviewed May 26, 2020

View reviewed changes

src/csharp/Grpc.Tools/build/_protobuf/Google.Protobuf.Tools.targets Outdated Show resolved Hide resolved

JunTaoLuo reviewed May 26, 2020

View reviewed changes

JunTaoLuo approved these changes May 26, 2020

View reviewed changes

_Protobuf_SelectFiles depends on _Protobuf_SetProtoRoot

d8e5237

jtattermusch added the kokoro:run label May 29, 2020

grpc-kokoro removed the kokoro:run label May 29, 2020

jtattermusch reviewed Jun 4, 2020

View reviewed changes

jtattermusch approved these changes Jun 5, 2020

View reviewed changes

jtattermusch merged commit 900699c into grpc:master Jun 5, 2020

Falco20019 deleted the grpc-tools-fix-cpp-cleanup branch June 29, 2020 11:53

Conversation

Falco20019 commented May 7, 2020

Uh oh!

jtattermusch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Falco20019 commented May 25, 2020

Uh oh!

Uh oh!

JunTaoLuo May 26, 2020

Choose a reason for hiding this comment

Uh oh!

Falco20019 May 26, 2020

Choose a reason for hiding this comment

Uh oh!

jtattermusch Jun 4, 2020

Choose a reason for hiding this comment

Uh oh!

Falco20019 Jun 4, 2020

Choose a reason for hiding this comment

Uh oh!

jtattermusch Jun 5, 2020

Choose a reason for hiding this comment

Uh oh!

JunTaoLuo left a comment

Choose a reason for hiding this comment

Uh oh!

Falco20019 commented May 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Falco20019 commented Jun 4, 2020

Uh oh!

jtattermusch commented Jun 4, 2020

Uh oh!

jtattermusch commented Jun 4, 2020

Uh oh!

jtattermusch left a comment

Choose a reason for hiding this comment

Uh oh!

Falco20019 commented Jun 4, 2020

Uh oh!

jtattermusch commented Jun 5, 2020

Uh oh!

jtattermusch commented Jun 5, 2020

Uh oh!

jtattermusch left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Falco20019 commented May 27, 2020 •

edited

Loading