-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
libxpm: add v3.5.17 #22197
libxpm: add v3.5.17 #22197
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
1dfe5a7
to
1e289dc
Compare
This comment has been minimized.
This comment has been minimized.
Conan v1 pipeline ✔️All green in build 7 (
Conan v2 pipeline ✔️
All green in build 7 ( |
src/RdFToP.c | ||
src/WrFFrP.c | ||
) | ||
target_include_directories(Xpm PRIVATE "${X11_INCLUDE_DIR}") | ||
target_link_libraries(Xpm PRIVATE "${X11_LIBRARY}") | ||
endif() | ||
|
||
if(WIN32) | ||
set_target_properties(Xpm PROPERTIES WINDOWS_EXPORT_ALL_SYMBOLS TRUE) |
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 know the usage of WINDOWS_EXPORT_ALL_SYMBOLS
is extended across several recipes, however, in recent dicussions we agreed to not exposing symbols if the library does not support it natively. So, in this case, I think we can just drop it and set shared as not supported for this recipe. Thanks 🙏
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.
This is equivalent to what the Autotools build system in the project would do under Windows. I understand the concern, but I don't think it is justified for a simple C library. I don't like disabling configurations unnecessarily as it will disable quite a few configurations downstream in other packages on CCI.
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.
The non-CMake build system in the project uses libtool to handle shared libs. From libtool docs:
With Microsoft tools, Libtool digs through the object files that make up the library, looking for non-static symbols to automatically export. I.e., Libtool with Microsoft tools tries to mimic the auto-export feature of contemporary GNU tools. It should be noted that the GNU auto-export feature is turned off when an explicit __declspec(dllexport) is seen. The GNU tools do this to not make more symbols visible for projects that have already taken the trouble to decorate symbols. There is no similar way to limit what symbols are visible in the code when Libtool is using Microsoft tools. In order to limit symbol visibility in that case you need to use one of the options -export-symbols or -export-symbols-regex.
In other words, it runs dumpbin
on the .o files and creates a .def file from the listed symbols. This is pretty much exactly what WINDOWS_EXPORT_ALL_SYMBOLS
accomplishes.
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.
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.
Approving this one from the discussion in the review and it being a C library
Also:
.def
file for Windows DLLs. It's not clear how it was generated andWINDOWS_EXPORT_ALL_SYMBOLS
should be ok for such a small library.*P.c
cannot be used on Windows.--enable-stat-zfile
feature that requiresgzip
to be present to automatically decompress.xpm.gz
files. Could be added later as an option.