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 preprocessor directives in function-like macros. #5392

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Malcolmnixon
Copy link

This PR fixes issue #5366 by reworking the preprocessor directives to surround the function-like macro rather than occurring inside the function-like macro parameter list.

This change allows the GDMP project to successfully build its windows target rather than failing with:
.\mediapipe/tasks/cc/core/task_api_factory.h(86): error C2121: '#': invalid character: possibly the result of a macro expansion

Copy link

google-cla bot commented May 8, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@GeorgeS2019
Copy link

@kuaashish
Thank you for making the necessary changes to make this PR possible.

Please help us to get this PR approved.
#5366 (comment)

@GeorgeS2019
Copy link

@Malcolmnixon
Gentle reminder. Do we need to keep this issue open?
#5366

@Malcolmnixon
Copy link
Author

Do we need to keep this issue open?

I would assume so - although that may be up to the ticketing policies of this project. I'm used to policies where an issue remains open until such time as a solution has been "merged" which fixes it. This PR is one possible solution; however the core team may request additional changes, or a different PR may get merged before this one which also fixes the issue - thus rendering this PR unnecessary.

@GeorgeS2019
Copy link

Thx for feedback

I will remind right actions to happen

@kuaashish
Copy link
Collaborator

Hi @schmidt-sebastian,

Could you please review this PR?

Thank you!!

@GeorgeS2019
Copy link

GeorgeS2019 commented May 10, 2024

@kuaashish
@schmidt-sebastian

Do seriously consider no more experimental Windows build

So the Unity and Godot communities will have the latest CI windows build without having someone (right now missing in ALL unity to deliver github CI action windows build) knowing how to debug and ensure successful windows build
[Mediapipe supports Windows] No more experimental build

@Malcolmnixon
Copy link
Author

Do seriously consider no more experimental Windows build

While this PR does allow the code to compile under Visual Studio, it's more about having the mediapipe code avoid use-cases undefined by the C/C++ specification, and thus being more compatible with different compilers.

A deeper discussion about official Windows platform support should probably occur on a separate issue, as there may be many other contributing factors (such as 3rd party dependencies, or spaces in filenames disrupting scripts) resulting in the mediapipe team deciding Windows support warrants being kept as experimental.

@GeorgeS2019
Copy link

GeorgeS2019 commented May 10, 2024

@Malcolmnixon
Thanks for investing lots of time in getting CI windows build using VS2019

There are many c++ projects attempting to do that. My overview impression is that NONE succeeded.

They remain in e.g. v0.10.9, due to one or two heros who managed to implement significant modifications to get it compile in e.g. VS20219

The Unity and TensorflowLite c# repos remain unsuccessful in implementing a CI to access the latest version.

I hope google team can address that so many projects can benefit IMMEDIATELY of the latest AI innovation coming from google.

@kuaashish
@schmidt-sebastian

As Malcom suggested. one step at a TIME. Looking forwards that this PR is merged.

@GeorgeS2019
Copy link

@schmidt-sebastian
Can you review?

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.

4 participants