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

[google-cloud-cpp] add new features #23056

Merged

Conversation

coryan
Copy link
Contributor

@coryan coryan commented Feb 11, 2022

Add two new GA features in google-cloud-cpp.

  • What does your PR fix?

N/A

  • Which triplets are supported/not supported? Have you updated the CI baseline?

No change

Yes.

  • If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

Yes.

@coryan coryan marked this pull request as ready for review February 11, 2022 20:58
@coryan
Copy link
Contributor Author

coryan commented Feb 11, 2022

I tested the new (and old) features locally, on Linux only.

Copy link
Contributor

@ras0219-msft ras0219-msft left a comment

Choose a reason for hiding this comment

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

LGTM, just let me know if you'd like to remove the protobuf/host dependency or not. Thanks as always!

"host": true
},
"protobuf",
{
Copy link
Contributor

Choose a reason for hiding this comment

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

While it doesn't harm anything, I don't think this host dependency on protobuf has any effect. I see that the portfile does do

vcpkg_add_to_path(PREPEND "${CURRENT_HOST_INSTALLED_DIR}/tools/grpc")

but it seems that any host version of protobuf must be being pulled in via some other mechanism (which should itself express that dependency).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting. I am Okay merging as-is. We will be adding more features in the next few weeks and that should give me plenty of motivation to make these things smaller.

Meanwhile: google-cloud-cpp depends both directly and indirectly on Protobuf. What is the recommended practice in such cases?

Just rely on the indirect dependency? For stable libraries that seems Okay, but it is prone to breaking a leaf if an intermediate dependency drops an ancestor dep.

Or are you saying that google-cloud-cpp already is taking advantage of the indirect dependency? If so, should I make other changes to express the direct dependency better?

Copy link
Contributor

Choose a reason for hiding this comment

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

What I mean is that it appears there is a direct dependency only on the target variant, not on the host variant. This is in contrast to grpc which clearly has a direct dependency on the host variant (based on the use of CURRENT_HOST_INSTALLED_DIR). Therefore I was suggesting that the host variant of protobuf be removed from the list since any dependency on it is an indirect dependency. It should be handled elsewhere by the component that has it as a direct dependency.

@JonLiu1993 JonLiu1993 self-assigned this Feb 14, 2022
@JonLiu1993 JonLiu1993 added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label Feb 14, 2022
@JonLiu1993
Copy link
Member

All features are tested successfully in the following triplet:

  • x86-windows
  • x64-windows
  • x64-windows-static

@JonLiu1993 JonLiu1993 added info:reviewed Pull Request changes follow basic guidelines and removed info:reviewed Pull Request changes follow basic guidelines labels Feb 14, 2022
@ras0219-msft ras0219-msft merged commit 9341313 into microsoft:master Feb 15, 2022
Jimmy-Hu added a commit to Jimmy-Hu/vcpkg that referenced this pull request Feb 15, 2022
[google-cloud-cpp] add new features (microsoft#23056)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants