Fix properties in Grpc.Tools#22896
Conversation
* Protobuf_ToolsOs * Protobuf_ToolsCpu * Protobuf_ProtocFullPath
| @@ -68,9 +68,9 @@ | |||
|
|
|||
| <PropertyGroup> | |||
| <!-- First try environment variable. --> | |||
There was a problem hiding this comment.
Comment should be updated to say that environment variables don't override explicitly set values
There was a problem hiding this comment.
Great catch! Would be good to know what is the intended behavior from @kkm000
Do we want env variables to overwrite props or do we want props to overwrite env? Dependent on that, we can also only overwrite the prop if env is not empty. This would also fix the bug of not being able to set it at all right now.
I‘m not sure if the use cases here. Maybe having it set in env for build servers? Then env should overwrite props.
There was a problem hiding this comment.
Maybe @jtattermusch also knows if we want to have ENV overwrite PROPS or the other way around to adjust the logic accordingly.
|
I'm actually not sure how important it is to use values from |
|
According to https://github.com/grpc/grpc/blob/master/src/csharp/BUILD-INTEGRATION.md#ill-investigate-that-when-i-have-time-i-just-want-to-run-protoc-as-i-did-before these are set and I assumed that I could also set them like most of the other props documented there. But it‘s only possible by setting the env or prop as One use case I was thinking of was redefining it on a build server by using a I don’t need it currently, so I‘m fine with keeping the current behavior. I was just wondering that these are the only 3 props that can’t be overwritten in the whole package and since But I do see your point that most users will never touch it. And those in need might also check the target file and find it out like I did. |
jtattermusch
left a comment
There was a problem hiding this comment.
As said, I'm not sure how useful this is, but what you're saying makes sense, so LGTM.
|
@apolcyn please add second LGTM. |
Protobuf_ToolsOs, Protobuf_ToolsCpu and Protobuf_ProtocFullPath are currently always overwritten by the environment variables without a checking-condition.
I added the checks so that it becomes possible to set them in the csproj as described (and expected) in the BUILD-INTEGRATION.md.
CC @jtattermusch @kkm000