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

[grpc] Fix x86-windows build issue #17276

Closed
wants to merge 5 commits into from
Closed

[grpc] Fix x86-windows build issue #17276

wants to merge 5 commits into from

Conversation

ykadowak
Copy link

I think this is a regression of b5bb151.
The original PR contains a lot of changes but I only understand a small part of it. So I am not sure if this is a right way to fix this.

@JonLiu1993 JonLiu1993 self-assigned this Apr 14, 2021
@JonLiu1993 JonLiu1993 added the category:port-bug The issue is with a library, which is something the port should already support label Apr 14, 2021
@JonLiu1993
Copy link
Member

@yusuke-kadowaki ,Thanks for your contribution,please run the vcpkg x-add-version grpc --overwrite-version command,then commit again

Copy link
Member

@JonLiu1993 JonLiu1993 left a comment

Choose a reason for hiding this comment

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

@yusuke-kadowaki ,please add "port-version" : 1 in the vcpkg.json

@JonLiu1993
Copy link
Member

The failures caused by popsift will be fixed by #17277.

@ghost
Copy link

ghost commented Apr 14, 2021

CLA assistant check
All CLA requirements met.

@ykadowak
Copy link
Author

@yusuke-kadowaki ,please add "port-version" : 1 in the vcpkg.json

Wait, which vcpkg.json? I edited the grpc one from "port-version" : 3 to 1, which does not seem right.

@JonLiu1993
Copy link
Member

@yusuke-kadowaki ,sorry!I made a mistake, I posted its comment on you when I was reviewing another pr, my fault

@ykadowak
Copy link
Author

ykadowak commented Apr 14, 2021

@JonLiu1993 Now I'm confused haha. I don't know why these tests are failing. Can you tell me what specifically you wanted me to do?

@dg0yt
Copy link
Contributor

dg0yt commented Apr 14, 2021

You changed the portfile, so you shall increment the port version. Than commit this change.
After this commit, let vcpkg register the new port version by vcpkg x-add-version grpc, and make another commit.
This is documented in the Versioning part of the mainainer's guide.

@ykadowak
Copy link
Author

@dg0yt I was misunderstanding the versioning. Thank you for the comment.

@ykadowak ykadowak marked this pull request as ready for review April 15, 2021 01:52
@JonLiu1993 JonLiu1993 added the info:reviewed Pull Request changes follow basic guidelines label Apr 15, 2021
@ras0219
Copy link
Contributor

ras0219 commented Apr 21, 2021

In order to avoid hard-coding so much about whether the target platform is executable, I've created an alternative PR #17424 which makes codegen a feature. You can then easily build it using vcpkg install grpc[codegen] or adding codegen to the list of required features in your manifest file.

@yusuke-kadowaki Could you try out that PR and let us know if it solves your issue?

@ykadowak
Copy link
Author

@ras0219
I think desired behavior when running vcpkg install grpc on Windows is building with CODEGEN feature ON for both x64-windows and x86-windows. That is because building for x86-windows on x64-windows is not a usual cross compiling. It used to work like that before b5bb151.

Anyway, vcpkg install grpc[codegen] worked on my end too.

@ras0219
Copy link
Contributor

ras0219 commented Apr 22, 2021

The problem with that assumption is that arm64 is becoming much more prominent as a platform for all three OS's (Windows, OSX, and Linux). I think it's much better to have a consistent story that works for everyone, rather than trying to build a bunch of case-by-case stuff that is likely to need changes or be fully scrapped in the future.

@ykadowak
Copy link
Author

ykadowak commented Apr 23, 2021

I understand that the hard-coding part is bad and needs fixed anyway.

The only concern is, as I mentioned, it would be a bit confusing for x86-windows developer I think. What if you add something like this to you PR #17424.

else if((TARGET_TRIPLET STREQUAL "x86-windows") AND (HOST_TRIPLET STREQUAL "x64-windows"))
set(gRPC_BUILD_CODEGEN ON)

Just my opinion. I'm closing this and leave it to your PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Grpc installed libraries difference between x64-windows and x86-windows
4 participants