-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
[gsl] fix gsl x64-linux-dynamic build by guarding win32 string replaces #31504
[gsl] fix gsl x64-linux-dynamic build by guarding win32 string replaces #31504
Conversation
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.
Related issues:
Without this, x64-linux-dynamic
fails on debug as follows:
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.
You shouldn't completely remove burning GSL_DLL
into the installed header.
ports/gsl/CMakeLists.txt
Outdated
if(BUILD_SHARED_LIBS) | ||
target_compile_definitions(gsl PRIVATE GSL_DLL) | ||
endif(BUILD_SHARED_LIBS) |
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.
... but this isn't.
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.
Thanks for correcting, my cmake knowledge isn't extensive. The section looks like this now:
file(READ gsl_types.h GSLTYPES_H)
if(WIN32)
if(BUILD_SHARED_LIBS)
target_compile_definitions(gsl PRIVATE GSL_DLL)
endif()
endif(WIN32)
file(WRITE ${CMAKE_CURRENT_BINARY_DIR}/gsl_types.h "${GSLTYPES_H}")
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.
Oh snap, I actually meant target_compile_definitions(gsl PRIVATE DLL_EXPORT)
@dg0yt is right, burning GSL_DLL
into this header is still needed.
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.
Oh snap, I actually meant
target_compile_definitions(gsl PRIVATE DLL_EXPORT)
@dg0yt is right, burningGSL_DLL
into this header is still needed.
No problem. Have made the change locally. However, the CI seems to point out the following problem downstream with mathgl:
error: building vcpkg-ci-mathgl:x86-windows failed with: CASCADED_DUE_TO_MISSING_DEPENDENCIES
due to the following missing dependencies:
mathgl[arma]:x86-windows
mathgl[core]:x86-windows
mathgl[examples]:x86-windows
mathgl[fltk]:x86-windows
mathgl[gif]:x86-windows
mathgl[glut]:x86-windows
mathgl[gsl]:x86-windows
mathgl[hdf5]:x86-windows
mathgl[jpeg]:x86-windows
mathgl[opengl]:x86-windows
mathgl[png]:x86-windows
mathgl[qt5]:x86-windows
mathgl[wx]:x86-windows
mathgl[zlib]:x86-windows
Can you give advice on how to resolve this? I guess it may be related to this
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.
It is really only one line which needs to changed or guarded.
CI is passing. |
@sjperkins, thanks for the PR, please also change the vcpkg.json file "port-version" from 2 to 3. |
Note: I will be converting your PR to draft status. When you respond, please revert to "ready for review". That way, I can be aware that you've responded since you can't modify the tags. |
Thanks for the merge |
./vcpkg x-add-version --all
and committing the result.