minio / minio-cpp

MinIO C++ Client SDK for Amazon S3 Compatible Cloud Storage
https://minio-cpp.min.io/
Apache License 2.0
127 stars 53 forks source link

Fixed a GetObject() function colliding with WinAPI macro #139

Closed kobalicek closed 2 months ago

kobalicek commented 5 months ago

Fixes https://github.com/minio/minio-cpp/issues/134

kobalicek commented 4 months ago

@balamurugana Ping :)

balamurugana commented 4 months ago

I am trying to reproduce the error with Github CI by PR https://github.com/minio/minio-cpp/pull/141. If I include windows.h, it works without this PR and including wingdi.h fails with other errors.

kobalicek commented 4 months ago

The failure doesn't seem to be caused by minio-cpp at all. I'm not a windows expert, but I think that since <wingdi.h> doesn't include any other headers it cannot be just included without including others first - most likely at least <windef.h>. WinAPI is old and usually people just include a super header <windows.h> to get most of the core stuff.

harshavardhana commented 3 months ago

PTAL @balamurugana

balamurugana commented 3 months ago

@harshavardhana We need a working reproducer of the issue. Please refer https://github.com/minio/minio-cpp/pull/141

kobalicek commented 3 months ago

That's a bad reproducer - create a fresh project and include and you will see the same failure.

The change is actually solid - it creates the right symbol (without A or W suffix) and then offers inlines in case was included. But it's transparent for users.

balamurugana commented 3 months ago

That's a bad reproducer - create a fresh project and include and you will see the same failure.

The change is actually solid - it creates the right symbol (without A or W suffix) and then offers inlines in case was included. But it's transparent for users.

@kobalicek Could you add below test case for this PR?

diff --git a/tests/tests.cc b/tests/tests.cc
index e06e1b3..866de1c 100644
--- a/tests/tests.cc
+++ b/tests/tests.cc
@@ -15,6 +15,11 @@
 //
 // SPDX-License-Identifier: Apache-2.0

+#ifdef _WIN32
+#define UNICODE
+#include <windows.h>  // To Test https://github.com/minio/minio-cpp/issues/134
+#endif
+
 #include <miniocpp/args.h>
 #include <miniocpp/client.h>
 #include <miniocpp/http.h>
harshavardhana commented 2 months ago

That's a bad reproducer - create a fresh project and include and you will see the same failure. The change is actually solid - it creates the right symbol (without A or W suffix) and then offers inlines in case was included. But it's transparent for users.

@kobalicek Could you add below test case for this PR?

diff --git a/tests/tests.cc b/tests/tests.cc
index e06e1b3..866de1c 100644
--- a/tests/tests.cc
+++ b/tests/tests.cc
@@ -15,6 +15,11 @@
 //
 // SPDX-License-Identifier: Apache-2.0

+#ifdef _WIN32
+#define UNICODE
+#include <windows.h>  // To Test https://github.com/minio/minio-cpp/issues/134
+#endif
+
 #include <miniocpp/args.h>
 #include <miniocpp/client.h>
 #include <miniocpp/http.h>

is this added ? PTAL @kobalicek

harshavardhana commented 2 months ago

@kobalicek ^^ PTAL in-case you missed.

Wiladams commented 2 months ago

Just for posterity, this is how I include "windows" in my app

include

define WIN32_LEAN_AND_MEAN

define NOMINMAX

define _WINSOCK_DEPRECATED_NO_WARNINGS 1

include