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

Removed export of methods from exported class (breaks windows DLL builds) #3601

Closed
wants to merge 1 commit into from

Conversation

iboB
Copy link
Contributor

@iboB iboB commented Nov 18, 2020

When building ImGui as a DLL on Windows there's a compilation error as Windows compilers don't allow giving a DLL interface to specific methods of a class which itself has a DLL interface. It should be one or the other.

@iboB
Copy link
Contributor Author

iboB commented Nov 18, 2020

perhaps CI jobs which build a shared library for different platforms should be added?

@ocornut
Copy link
Owner

ocornut commented Nov 18, 2020

Hello,

Thanks for the PR. There's already a CI job for this:

https://github.com/ocornut/imgui/runs/1417199898?check_suite_focus=true

See under Windows > Build example_null (as DLL)
Before merging this I would like this test to be reworked to reflect the issue you are seeing. Can you clarify your compiler, compile option, and try to create a separate PR (without this fix) that would rightly trigger an error/warning? Then once we have it we can merge both PR.

Thank you!

@ocornut
Copy link
Owner

ocornut commented Nov 18, 2020

That specific test is doing:

  echo '#ifdef _EXPORT'                                  >  example_single_file.cpp
  echo '#  define IMGUI_API __declspec(dllexport)'       >> example_single_file.cpp
  echo '#else'                                           >> example_single_file.cpp
  echo '#  define IMGUI_API __declspec(dllimport)'       >> example_single_file.cpp
  echo '#endif'                                          >> example_single_file.cpp
  echo '#define IMGUI_IMPLEMENTATION'                    >> example_single_file.cpp
  echo '#include "misc/single_file/imgui_single_file.h"' >> example_single_file.cpp
  cl.exe /D_USRDLL /D_WINDLL /D_EXPORT /I. example_single_file.cpp /LD /FeImGui.dll /link
  cl.exe /I. ImGui.lib /Feexample_null.exe examples/example_null/main.cpp

@rokups
Copy link
Contributor

rokups commented Nov 18, 2020

I am surprised this would produce an error, as neither our CI nor my own projects building in this configuration have any issues.

With that said, i would say change is still a good one, as it removes redundant API linkage specifiers. One on the struct does the job for all applicable methods.

@iboB
Copy link
Contributor Author

iboB commented Nov 18, 2020

https://github.com/iboB/imgui-dll/runs/1420026650?check_suite_focus=true#step:4:13

Visual Studio 2019 (but I'm 99% sure I'll get the same errors on other versions)

Just a dll with imgui.cpp

@iboB
Copy link
Contributor Author

iboB commented Nov 18, 2020

To make a PR without this fix which triggers the error would take me a bit more time, as I'd have to study the current CI configs, but sure. I'll have time for it tomorrow and I'll do it in 18-20 hours if no one else has done it by then

@iboB
Copy link
Contributor Author

iboB commented Nov 18, 2020

by they way on cmd echo doesn't work like echo on bash
echo 'foo' produces the literal 'foo' with quotes. So at a glance, the code you provided should produce the file:

'#ifdef _EXPORT'                                  
'#  define IMGUI_API __declspec(dllexport)'       
'#else'                                           
'#  define IMGUI_API __declspec(dllimport)'       
'#endif'                                          
'#define IMGUI_IMPLEMENTATION'                    
'#include "misc/single_file/imgui_single_file.h"' 

Unless GH actions provides some other magic. But the strange thing is that this is not valid C++. It should fail with different compilation errors. So for now I hace no idea what's going on with the CI

@iboB
Copy link
Contributor Author

iboB commented Nov 18, 2020

Ok. This got me intrigued and instead of sleeping I made some experiments.

So it turns out that for some reason none of the code after

https://github.com/ocornut/imgui/blob/master/.github/workflows/build.yml#L74

... gets executed. Calling vcvarsall simply terminates the step

Thus, the bad echo's don't even get executed. If they were executed, they would produce an error like "too many characters in char literal" (see prev comment)

I have continued experimenting in my repo from above, and no matter what I add after the vcvarsall call, it always completes sucessfully.

I'll come up with a solution tomorrow (or... you know... today if I can't sleep because of this)

@iboB
Copy link
Contributor Author

iboB commented Nov 18, 2020

spoiler: it was today 😞

@ocornut
Copy link
Owner

ocornut commented Nov 18, 2020

Merged jointly in fcc2b71, thank you!

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