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

Remove protoc conditional properties #4817

Merged
merged 2 commits into from Jun 25, 2018

Conversation

ObsidianMinor
Copy link
Contributor

Copy link

@kkm000 kkm000 left a comment

Choose a reason for hiding this comment

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

LGTM besides this one.

@@ -1,16 +1,11 @@
<Project>
<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">>
Copy link

Choose a reason for hiding this comment

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

Duplicate >>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@kkm000
Copy link

kkm000 commented Jun 23, 2018

LGTM, thanks.

@kkm000
Copy link

kkm000 commented Jun 23, 2018

@xfxyjwf, @anandolee, please merge this at your earliest convenience, so that broken script on master won't get released in a nuget by mistake.

@anandolee
Copy link
Contributor

anandolee commented Jun 25, 2018

3.6.0 is already released last week. Note our release is not from master that #4648 should not included.

But we are appreciate and can you help to try if 3.6.0 release works well? Thanks
https://www.nuget.org/packages/Google.Protobuf/
https://www.nuget.org/packages/Google.Protobuf.Tools/

Will merge this change after tests

@anandolee anandolee merged commit 3d603f4 into protocolbuffers:master Jun 25, 2018
@kkm000
Copy link

kkm000 commented Jun 26, 2018 via email

@anandolee
Copy link
Contributor

@kkm000, how's the tests?

@kkm000
Copy link

kkm000 commented Jul 2, 2018

Hi @anandolee, sorry for the delay. I pulled the released package that you linked to, and the Tools package does not contain any .props/.targets files, so it is as good as all the previous releases are. There is nothing to test, strictly speaking, as the package does not attempt any MSBuild integration at all, just drops the packaged files.

So it's only the master branch that currently contains the breaking change. It's good that the change #4648 did not make it into that build! :)

Would you be willing to include pre-release packages in your release workflow? That may give you an early alert from volunteering users if something gets inconspicuously broken.

@anandolee
Copy link
Contributor

Thank you @kkm000. We used to have pre-release steps to wait early alert from users but almost no response every time so we remove the step to save our time (it is a large time cost for us). Our current solution is if a breaking change is released, we will release a minor version for fix

@kkm000
Copy link

kkm000 commented Jul 3, 2018

@anandolee, yes, I see, and admit I am guilty of that too! When I dropped out of Google back in 2014 and was selecting an RPC communication framework for servers in our current company (C# clients, C++ servers), I naturally went with gRPC and protobuf even though they were in the early alpha, as I knew them well (not exactly gRPC but its predecessor, but protobuf is same) and was certain that they will reach maturity in the public release. And I never gave any feedback to the project. My excuse is they just worked, and when they did not, the pace of change was quite high so I was finding that the next release already fixed it... I think that happened only once though.

I am very happy with this packaging change, as it tremendously helps locating the tools dropped by the package. With Microsoft's new approach to NuGet, the packages are now stored under user's common directory (~/.nuget on Linux, and an equivalent under Windows which is harder to locate than just ~/.nuget, unfortunately), while previously they were cached per-project. This made it much more cumbersome to find where the files are, as a simple project-relative path does not work any more.

I think we are totally on the same page. If you feel this packaging change warrants a pre-release package, feel free to do that and please tag me on GitHub so I do not miss the message (my phone buzzes for tagged mentions!), and I'll verify it on all OSes and environments I have here. Up to you entirely, just to let you know I'm in the loop! :)

I should mention also the PR with MSBuild integration for grpc/grpc#13207 (/cc @jtattermusch) that is currently under review. While writing the code for build tasks and MSBuild scripts, I always kept in mind the possible future splitting of this integration scaffolding into two parts. As is, all build integration in this package is going into Grpc.Tools. Ideally, however, most of it should be in Protobuf.Tools, where it really belongs, and only the gRPC plugin executable binaries and some MSBuild props and targets should remain in Grpc.Tools, which would depend on Protobuf.Tools for the rest of integration (and, naturally, protoc binaries and standard proto imports). The main difficulty that I perceive is that such a change should be a well-orchestrated effort of both your and gRPC teams. It is indeed possible to release the first package with Grpc.Tools, and reshuffle the packaging later, after we get it stable, and that's the current plan to the best of my understanding. The main requirement here (or, at the least, a very desirable way to go on) is that after this split of the build tools from Grpc.Tools to Protobuf.Tools, new release packages for both projects should be dropped at the same time for the initial release, as they would be quite tightly coupled at this moment. I'd like to ask you for your opinion on this plan, and would you be willing to eventually arrive at the point of protoc support being as well integrated into the .NET ecosystem as other compilers and code generators are.

This is the draft documentation I wrote for the integration services, which is in a separate CL grpc/grpc#15754. I am dropping the link here hoping it might make it easier for you to see the direction in which this is all going.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants