Windows: lib import/export macros

OgreTransporter commented

2022-11-09 05:46:19 -06:00

(Migrated from github.com)

github.com/libusb/hidapi@2b70208fea/hidapi/hidapi.h (L33)
This does not make sense. dllexport is normally always used in combination with dllimport (https://learn.microsoft.com/en-us/cpp/cpp/dllexport-dllimport).

https://github.com/libusb/hidapi/blob/2b70208feabeb4cb524dff58c450af63544790bc/hidapi/hidapi.h#L33 This does not make sense. `dllexport` is normally always used in combination with `dllimport` (https://learn.microsoft.com/en-us/cpp/cpp/dllexport-dllimport).

Youw commented

2022-11-09 06:53:37 -06:00

(Migrated from github.com)

"does not make" doesn't mean it is broken.
~~I remember 0 complains about this part.~~ UPD: there are issues - more details below.
And I personally use HDIAPI both as a shared and as a static library on multiple platforms, including Windows.

Please share a specific use-case where the old behavior has compilation/runtime issues. If there is such - we'll try to find a backward-compatible solution for all build systems.

"does not make" doesn't mean it is broken. ~~I remember 0 complains about this part.~~ UPD: there are issues - more details below. And I personally use HDIAPI both as a shared and as a static library on multiple platforms, including Windows. Please share a specific use-case where the old behavior has compilation/runtime issues. If there is such - we'll try to find a backward-compatible solution for all build systems.

OgreTransporter commented

2022-11-10 01:11:53 -06:00

(Migrated from github.com)

I never claimed that errors occur. But I can also argue the other way around, that just because you haven't had an error yet, it doesn't always have to work. After all, the Windows API provides for the combination of dllimport and dllexport. This is a general improvement suggestion to make the code cleaner. Many libraries like Boost (https://github.com/boostorg/system/blob/develop/include/boost/system/config.hpp) use this kind of macro mechanism to implement the Windows API correctly.

I never claimed that errors occur. But I can also argue the other way around, that just because you haven't had an error yet, it doesn't always have to work. After all, the Windows API provides for the combination of `dllimport` and `dllexport`. This is a general improvement suggestion to make the code cleaner. Many libraries like Boost (https://github.com/boostorg/system/blob/develop/include/boost/system/config.hpp) use this kind of macro mechanism to implement the Windows API correctly.

Youw commented

2022-11-10 06:38:12 -06:00

(Migrated from github.com)

You're perfectly right about how things should be in the ideal world.
And I'm definitely considering making it right for HIDAPI v1+.

But right now - my strong position is: "don't try to fix what is not broken". And the #480 breaks all windows builds on our CI.
And our CI is the kind of "golden sample" for those who seek to start using HIDAPI or a reflection of how HIDAPI is already used by many others.

You're perfectly right about how things should be in the ideal world. And I'm definitely considering making it right for HIDAPI v1+. But right now - my strong position is: "don't try to fix what is not broken". And the #480 breaks all windows builds on our CI. And our CI is the kind of "golden sample" for those who seek to start using HIDAPI or a reflection of how HIDAPI is already used by many others.

YgorSouza commented

2023-02-13 14:31:07 -06:00

(Migrated from github.com)

"does not make" doesn't mean it is broken. I remember 0 complains about this part. And I personally use HDIAPI both as a shared and as a static library on multiple platforms, including Windows.

Please share a specific use-case where the old behavior has compilation/runtime issues. If there is such - we'll try to find a backward-compatible solution for all build systems.

For what it's worth, I did have problems with that. I linked HIDAPI statically into my DLL, and then noticed that all of its functions were accessible and visible with for example dumpbin /exports. Then I looked at the header and saw that there was no way to disable that, so I just copied the header to my project and edited out the dllexport. Now I am planning on using the hidapi Rust crate, which uses this repo as a submodule, and I realized I'll have to fork this repo and then fork the crate repo to make this same change. That #ifndef HIDAPI_STATIC guard would simplify this a lot.

As for the dllimport, Microsoft says it's optional, and only serves to make the compiler generate "more efficient" code, whatever that means (source). So maybe it's not worth it if it is a breaking change. Maybe change the ifdef to an ifndef? Then for most people it changes nothing, and people who want to get that "more efficient" code can just define the symbol.

> "does not make" doesn't mean it is broken. I remember 0 complains about this part. And I personally use HDIAPI both as a shared and as a static library on multiple platforms, including Windows. > > Please share a specific use-case where the old behavior has compilation/runtime issues. If there is such - we'll try to find a backward-compatible solution for all build systems. For what it's worth, I did have problems with that. I linked HIDAPI statically into my DLL, and then noticed that all of its functions were accessible and visible with for example `dumpbin /exports`. Then I looked at the header and saw that there was no way to disable that, so I just copied the header to my project and edited out the dllexport. Now I am planning on using the hidapi Rust crate, which uses this repo as a submodule, and I realized I'll have to fork this repo and then fork the crate repo to make this same change. That `#ifndef HIDAPI_STATIC` guard would simplify this a lot. As for the dllimport, Microsoft says it's optional, and only serves to make the compiler generate "more efficient" code, whatever that means ([source](https://learn.microsoft.com/en-us/cpp/build/importing-into-an-application-using-declspec-dllimport?view=msvc-170)). So maybe it's not worth it if it is a breaking change. Maybe change the `ifdef` to an `ifndef`? Then for most people it changes nothing, and people who want to get that "more efficient" code can just define the symbol.

Youw commented

2023-02-13 14:46:11 -06:00

(Migrated from github.com)

all of its functions were accessible and visible with for example dumpbin /exports. Then I looked at the header and saw that there was no way to disable that

That is a very good case.

To be able to workaround cases like that I suggest:

 #ifdef _WIN32
+   #ifndef HID_API_EXPORT
       #define HID_API_EXPORT __declspec(dllexport)
+   #endif
       #define HID_API_CALL
 #else
+   #ifndef HID_API_EXPORT
       #define HID_API_EXPORT /**< API export macro */
+   #endif
       #define HID_API_CALL /**< API call macro */
 #endif

and a corresponding define for CMake build of the static library

that is not perfect (not even close) but should be good-enough for v0

> all of its functions were accessible and visible with for example dumpbin /exports. Then I looked at the header and saw that there was no way to disable that That is a very good case. To be able to workaround cases like that I suggest: ```diff #ifdef _WIN32 + #ifndef HID_API_EXPORT #define HID_API_EXPORT __declspec(dllexport) + #endif #define HID_API_CALL #else + #ifndef HID_API_EXPORT #define HID_API_EXPORT /**< API export macro */ + #endif #define HID_API_CALL /**< API call macro */ #endif ``` and a corresponding define for CMake build of the static library --- that is not perfect (not even close) but should be good-enough for `v0`

YgorSouza commented

2023-02-13 16:04:36 -06:00

(Migrated from github.com)

Maybe not the most intuitive, but at least it gives users more flexibility to do what they need without having to change the code itself, and doesn't break anything that currently works. I'd say it's a decent enough compromise.

SamuelC0507 commented

2023-02-24 00:07:25 -06:00

(Migrated from github.com)

Hi @Youw ,
I have a similar use case of HIDAPI as @YgorSouza 's.

On my end, target_compile_definitions(hidapi_winapi PUBLIC HID_API_EXPORT) in \windows\CMakeLists.txt results in HID_API_EXPORT being defined as 1, causing every function declaration in hidapi.h resulting in error C2059: syntax error : 'constant' during compilation. Not sure if you guys ever ran into this problem?

Maybe something like target_compile_definitions(hidapi_winapi PUBLIC HID_API_STATIC) in \windows\CMakeLists.txt, and

#if defined(_WIN32) && !defined(HID_API_STATIC)
    #define HID_API_EXPORT __declspec(dllexport)
    #define HID_API_CALL
#else
    #define HID_API_EXPORT /**< API export macro */
    #define HID_API_CALL /**< API call macro */
#endif

in hidapi.h would be nice?

Hi @Youw , I have a similar use case of HIDAPI as @YgorSouza 's. On my end, `target_compile_definitions(hidapi_winapi PUBLIC HID_API_EXPORT)` in \windows\CMakeLists.txt results in `HID_API_EXPORT` being defined as `1`, causing every function declaration in hidapi.h resulting in `error C2059: syntax error : 'constant'` during compilation. Not sure if you guys ever ran into this problem? Maybe something like `target_compile_definitions(hidapi_winapi PUBLIC HID_API_STATIC)` in \windows\CMakeLists.txt, and ``` #if defined(_WIN32) && !defined(HID_API_STATIC) #define HID_API_EXPORT __declspec(dllexport) #define HID_API_CALL #else #define HID_API_EXPORT /**< API export macro */ #define HID_API_CALL /**< API call macro */ #endif ``` in hidapi.h would be nice?

Youw commented

2023-02-24 08:40:33 -06:00

(Migrated from github.com)

HID_API_EXPORT being defined as 1

Very interesting. I haven't seen a compiler that would do somehting like that.
And one of the onces on our CI behaves that way.

And only now I took attention into CMake documentation that actually describes that behavior:

Note that many compilers treat -DFOO as equivalent to -DFOO=1...

Care to share your compiler and its version?
Just out of my curiosity.

&& !defined(HID_API_STATIC)

Yes, I guess that should work.
Maybe a little less flexible solution (as it doesn't give the ability to define HID_API_EXPORT to something else (e.g. __attribute__((visibility("default")))), but as a solution to this problem - more than acceptible.

> `HID_API_EXPORT` being defined as `1` Very interesting. I haven't seen a compiler that would do somehting like that. And one of the onces on our CI behaves that way. And only now I took attention into [CMake documentation](https://cmake.org/cmake/help/latest/command/target_compile_definitions.html) that actually describes that behavior: > Note that many compilers treat `-DFOO` as equivalent to `-DFOO=1`... Care to share your compiler and its version? Just out of my curiosity. --- > && !defined(HID_API_STATIC) Yes, I guess that should work. Maybe a little less flexible solution (as it doesn't give the ability to define `HID_API_EXPORT` to something else (e.g. `__attribute__((visibility("default")))`), but as a solution to this problem - more than acceptible.

OgreTransporter commented

2023-02-25 04:38:36 -06:00

(Migrated from github.com)

Have a look at https://github.com/OGRECave/ogre-next/blob/master/OgreMain/include/OgrePlatform.h as a full example for static and dynamic with export and import.

A summary is available at https://github.com/Framstag/libosmscout/blob/master/libosmscout/include/osmscout/CoreImportExport.h.

Even in the GCC documentation the usage is documented like this https://gcc.gnu.org/wiki/Visibility.

The original idea of the proposal was to adapt the import-export API correctly for all cases (static, export, import). Since the simple macro solution is apparently still very unpopular, there is a second way. Simply remove the macro completely and supply a module definition file (https://learn.microsoft.com/en-us/cpp/build/reference/module-definition-dot-def-files?view=msvc-170) for Windows. In CMake it is then relatively simple, everything is identical, only under the Windows DLL this file must be passed to the compiler. The disadvantage of this way is that it only works with Visual Studio and clang under Windows. GCC cannot do this. If you really want to support all compilers in all directions, then you should implement the macro variant.

Have a look at https://github.com/OGRECave/ogre-next/blob/master/OgreMain/include/OgrePlatform.h as a full example for static and dynamic with export and import. A summary is available at https://github.com/Framstag/libosmscout/blob/master/libosmscout/include/osmscout/CoreImportExport.h. Even in the GCC documentation the usage is documented like this https://gcc.gnu.org/wiki/Visibility. The original idea of the proposal was to adapt the import-export API correctly for all cases (static, export, import). Since the simple macro solution is apparently still very unpopular, there is a second way. Simply remove the macro completely and supply a module definition file (https://learn.microsoft.com/en-us/cpp/build/reference/module-definition-dot-def-files?view=msvc-170) for Windows. In CMake it is then relatively simple, everything is identical, only under the Windows DLL this file must be passed to the compiler. The disadvantage of this way is that it only works with Visual Studio and clang under Windows. GCC cannot do this. If you really want to support all compilers in all directions, then you should implement the macro variant.

Youw commented

2023-02-25 05:37:06 -06:00

(Migrated from github.com)

I am aware of the the "perfect" solution using macros.
My position about this stands: until v1 - all changes has to be backward compatible with existing usages of HIDAPI (current CI maybe not perfect - but good-enough example of such).

And no changes to autotools - those are deprecated, and should stay untouched until removed in v1 (unless obvious bugs, which I'm not aware of).

I am aware of the the "perfect" solution using macros. My position about this stands: until v1 - all changes has to be backward compatible with existing usages of HIDAPI (current CI maybe not perfect - but good-enough example of such). And no changes to autotools - those are deprecated, and should stay untouched until removed in v1 (unless obvious bugs, which I'm not aware of).

SamuelC0507 commented

2023-02-26 10:07:23 -06:00

(Migrated from github.com)

@Youw , my cl version is 14.29.30133, came with Visual Studio 2019 Professional.

Maybe a little less flexible solution (as it doesn't give the ability to define HID_API_EXPORT to something else

My bad I was just thinking of a quick fix and didn't give it a thorough enough thought, I'm sure you would come up with a better way to handle this issue at V1 release, looking forward to it!

@Youw , my cl version is 14.29.30133, came with Visual Studio 2019 Professional. > Maybe a little less flexible solution (as it doesn't give the ability to define HID_API_EXPORT to something else My bad I was just thinking of a quick fix and didn't give it a thorough enough thought, I'm sure you would come up with a better way to handle this issue at V1 release, looking forward to it!

YgorSouza commented

2023-03-02 12:06:14 -06:00

(Migrated from github.com)

Hi @Youw , I have a similar use case of HIDAPI as @YgorSouza 's.

On my end, target_compile_definitions(hidapi_winapi PUBLIC HID_API_EXPORT) in \windows\CMakeLists.txt results in HID_API_EXPORT being defined as 1, causing every function declaration in hidapi.h resulting in error C2059: syntax error : 'constant' during compilation. Not sure if you guys ever ran into this problem?

Maybe something like target_compile_definitions(hidapi_winapi PUBLIC HID_API_STATIC) in \windows\CMakeLists.txt, and
#if defined(_WIN32) && !defined(HID_API_STATIC)
    #define HID_API_EXPORT __declspec(dllexport)
    #define HID_API_CALL
#else
    #define HID_API_EXPORT /**< API export macro */
    #define HID_API_CALL /**< API call macro */
#endif
in hidapi.h would be nice?

I hadn't tested until now, since I haven't touched this part of my code in a while, but indeed I got the same problem. One solution is to define the macro to something explicitly, so it doesn't get implicitly set to 1. This is done in the other empty definitions in the header file itself.

target_compile_definitions(hidapi PUBLIC HID_API_EXPORT=/**/)

> Hi @Youw , I have a similar use case of HIDAPI as @YgorSouza 's. > > On my end, `target_compile_definitions(hidapi_winapi PUBLIC HID_API_EXPORT)` in \windows\CMakeLists.txt results in `HID_API_EXPORT` being defined as `1`, causing every function declaration in hidapi.h resulting in `error C2059: syntax error : 'constant'` during compilation. Not sure if you guys ever ran into this problem? > > Maybe something like `target_compile_definitions(hidapi_winapi PUBLIC HID_API_STATIC)` in \windows\CMakeLists.txt, and > > ``` > #if defined(_WIN32) && !defined(HID_API_STATIC) > #define HID_API_EXPORT __declspec(dllexport) > #define HID_API_CALL > #else > #define HID_API_EXPORT /**< API export macro */ > #define HID_API_CALL /**< API call macro */ > #endif > ``` > > in hidapi.h would be nice? I hadn't tested until now, since I haven't touched this part of my code in a while, but indeed I got the same problem. One solution is to define the macro to something explicitly, so it doesn't get implicitly set to 1. This is done in the other empty definitions in the header file itself. ``` target_compile_definitions(hidapi PUBLIC HID_API_EXPORT=/**/) ```

👍 1

Youw commented

2023-03-06 06:31:58 -06:00

(Migrated from github.com)

The only thing I don't like about this aproach, that the /**/ would have to be escaped for a specific command prompt/terminal later for compiler to be executed.
CMake usually does a decent job doing so, but it has to be done carefully and tested in all environmnets.

I prefer not to pass "weird" symbols into command-line when I can.

The only thing I don't like about this aproach, that the `/**/` would have to be escaped for a specific command prompt/terminal later for compiler to be executed. CMake usually does a decent job doing so, but it has to be done carefully and tested in all environmnets. I prefer not to pass "weird" symbols into command-line when I can.

Rows
Columns

Windows: lib import/export macros #480