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

Azure Enable GRPC for Win32 #95

Merged
merged 6 commits into from
Mar 9, 2020
Merged

Azure Enable GRPC for Win32 #95

merged 6 commits into from
Mar 9, 2020

Conversation

RobertBColton
Copy link
Contributor

@RobertBColton RobertBColton commented Mar 8, 2020

Enable GRPC client for the Azure build on Win32.
https://cmake.org/cmake/help/latest/module/CMakeDependentOption.html#module:CMakeDependentOption
Also fix the option itself by removing PARENT_SCOPE, which was actually setting the variable a scope above the current scope and thus causing it to not actually build the server plugin.
https://cmake.org/cmake/help/v3.3/command/set.html

I tested the artifact produced from this build and it does indeed have the GRPC client built in. However, when launched from MSYS2 it doesn't seem to launch emake like my local build does. Although, if emake is manually launched, the Azure artifact will still communicate with it.

That just means this pull request is correct and we just need to work out yet how we expect people to launch this thing.

Enable GRPC client for the Azure build.
Copy link
Member

@JoshDreamland JoshDreamland left a comment

Choose a reason for hiding this comment

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

How does this not break everything? Did your previous changes actually fix GRPC?

@RobertBColton
Copy link
Contributor Author

Actually, it's just allegedly broke on Linux, or specifically fundies machine. As I said above, I tested the artifact and it's working over here on Windows. Despite that, fundies never actually disabled GRPC from the CMake/Azure build, he disabled it from the pro file which he and I use locally. All I've basically done now is made an option to turn it on or off easily, with no difference from how fundies left it a few days ago.

@RobertBColton
Copy link
Contributor Author

RobertBColton commented Mar 9, 2020

Ok fundies wants a Windows check around this and to only turn it on for Windows right now. I had it backwards, I use the pro file, he uses CMake himself locally.

Edit: Found a way to do it.
https://cmake.org/cmake/help/latest/module/CMakeDependentOption.html#module:CMakeDependentOption

@RobertBColton RobertBColton changed the title GRPC Azure Enable GRPC Azure Enable for Win32 Mar 9, 2020
@RobertBColton RobertBColton changed the title GRPC Azure Enable for Win32 Azure Enable GRPC for Win32 Mar 9, 2020
@RobertBColton
Copy link
Contributor Author

RobertBColton commented Mar 9, 2020

Alright, I added the check to only enable it for Win32 right now and it seems to work. I tested all 3 of the Windows releases and double checked that ServerPlugin was not getting built on Arch Linux like fundies wanted me to. With that said, I am merging this as a success!

@RobertBColton RobertBColton merged commit d7c4ea2 into master Mar 9, 2020
@RobertBColton RobertBColton deleted the grpc-azure-enable branch March 9, 2020 07:45
@fundies
Copy link
Contributor

fundies commented Mar 9, 2020

Its broken on josh's machine too las we checked so its not just a local issue for me

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

Successfully merging this pull request may close these issues.

3 participants