-
Notifications
You must be signed in to change notification settings - Fork 501
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
Ability to compile using MinGW build tools #2114
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for pytorch-fbgemm-docs canceled.
|
Hi @Kreijstal! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
bench/CMakeLists.txt
Outdated
@@ -65,6 +65,10 @@ macro(add_benchmark BENCHNAME) | |||
if (${BLAS_FOUND}) | |||
target_compile_options(${BENCHNAME} PRIVATE "-DUSE_BLAS") | |||
target_link_libraries(${BENCHNAME} "${BLAS_LIBRARIES}") | |||
if (DEFINED ENV{MINGW_PREFIX}) | |||
# Include the directory using the value of the environment variable | |||
include_directories("$ENV{MINGW_PREFIX}/include/openblas") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MINGW_PREFIX is a private environment variable in msys2. It should not be used in any other repository.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahh, I think I added it because I couldn't get openblas to work, thank you, I'll update it and fix it
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
to compile you will do it like this for mingw: cmake -G"MSYS Makefiles" -DFBGEMM_LIBRARY_TYPE=shared -DBLAS_INCLUDE_DIRS="$MINGW_PREFIX/include/openblas" .. I hope this is okay
@@ -244,7 +252,7 @@ if(NOT TARGET asmjit) | |||
|
|||
#build asmjit | |||
#For MSVC shared build, asmjit needs to be shared also. | |||
if (MSVC AND (FBGEMM_LIBRARY_TYPE STREQUAL "shared")) | |||
if ((MSVC OR MINGW) AND (FBGEMM_LIBRARY_TYPE STREQUAL "shared")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MSVC or MINGW is probably same as WIN32.
@@ -62,6 +63,11 @@ macro(add_gtest TESTNAME) | |||
target_compile_definitions(${TESTNAME} PRIVATE FBGEMM_STATIC) | |||
endif() | |||
else(MSVC) | |||
if(MINGW) | |||
#if (FBGEMM_LIBRARY_TYPE STREQUAL "static") | |||
target_compile_definitions(${TESTNAME} PRIVATE FBGEMM_STATIC) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume the macro should not be defined for all cases. That static library condition should be present, right?
Could you let me know what version of msys2 gcc you are using, and how to install them on a Windows machine? |
The installation procedure is documented here https://www.msys2.org/ |
The reason I asked this question is because the instructions at msys2 don't seem to apply well to the Windows CI runners. I've been trying to integrate MinGW builds into GitHub CI over at #2134, but compilation is failing at the linking stage (see here). Could you see if you can integrate the changes from that PR into this PR? |
Sure, I can work on that, do you mind if I use this helper? |
That should be fine, yes. |
please forgive the delays, I have lots of uni stuff at the moment, I'll catch up when I can :) |
In this pull request I have done some code changes to allow mingw to build FBGEMM, everything builds and links, tests and benchmarks too. fix #2113
This is the build procedure
I had trouble setting some flags for the tests, so I hardcoded
FBGEMM_STATIC
under mingw builds, I only did this to prove the point that mingw supports this compilation, and runs tests, however I lack the skills to edit the cmake so that the compilationjust works
, linkage error only happens when building the tests.These are the result of the tests
I am open for feedback and suggestions.
In case you're curious in the previous commit, you can also build using the shared library of asmjit that you can get with your msys distribution, but this probably polutes the project, so I took it out.