Enabling exporting symbols on windows #1035
Conversation
Small fix to allow for the built library to be used in windows pytorch#728
|
@peterjc123 could you have a look? |
Codecov Report
@@ Coverage Diff @@
## master #1035 +/- ##
=======================================
Coverage 63.31% 63.31%
=======================================
Files 65 65
Lines 5152 5152
Branches 772 772
=======================================
Hits 3262 3262
Misses 1666 1666
Partials 224 224Continue to review full report at Codecov.
|
CMakeLists.txt
Outdated
| @@ -8,6 +8,7 @@ file(GLOB_RECURSE HEADERS torchvision/csrc/vision.h) | |||
| file(GLOB_RECURSE MODELS_HEADERS torchvision/csrc/models/*.h) | |||
| file(GLOB_RECURSE MODELS_SOURCES torchvision/csrc/models/*.h torchvision/csrc/models/*.cpp) | |||
|
|
|||
| set(CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS ON) | |||
There was a problem hiding this comment.
Well, I have to say that enabling CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS is not the best option that we should take. Because MSVC has a hard limit on the numbers of the exported symbols, which is 65535. It may be used temporarily as a workground, but I don't think we should get this in for the following reasons:
- There is no CI test on Windows, so it will easily get broken at some time. (And at that time, it will be hard to fix.)
- The c++ codebase in this repo is not so big. So adding annotation is possible.
There was a problem hiding this comment.
Here is how we fix that in pytorch/pytorch: pytorch/pytorch#9491.
There was a problem hiding this comment.
Ah no problem, makes sense. Is there any preference on the naming of the macro, something like
#define TV_CPP_API
since the lib being built is called torchvision?
There was a problem hiding this comment.
@philipNoonan Maybe VISION_API is a good option. No need to add CPP into the name because it's c++ only.
There was a problem hiding this comment.
LGTM, but waiting for @peterjc123 thumbs up before merging.
| #define VISION_GENERAL_H | ||
|
|
||
| #ifdef _WIN32 | ||
| #define VISION_API __declspec(dllexport) |
There was a problem hiding this comment.
Hmm, it seemed to work locally - it built and the convertmodels sample ran as expected. I've added the dllimport now, and it also works, although I had to remove a spurious VISION_API call that I had put in a definition in resnet.h
|
Thanks a lot! |
Small fix to allow for the built library to be used in windows #728