-
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
[bdwgc] New port #6405
[bdwgc] New port #6405
Conversation
Hi @NNemec, thanks for the new port. Here are the test results from the current CI system:
|
As of right now, the port builds successfully on Windows and Linux. I'm a bit sceptical that the Linux build works properly, so I'll do some manual testing when I have time. |
Great! I got UWP building locally once and will try to integrate it within the next days. |
ports/bdwgc/portfile.cmake
Outdated
endif() | ||
|
||
if (NOT VCPKG_CMAKE_SYSTEM_NAME OR VCPKG_CMAKE_SYSTEM_NAME STREQUAL "WindowsStore") | ||
file(INSTALL "${CURRENT_BUILDTREES_DIR}/${TARGET_TRIPLET}-rel/${LIBNAME}" DESTINATION ${CURRENT_PACKAGES_DIR}/lib RENAME gc${STATIC_LIB_SUFFIX}) |
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.
We should use the upstream names here as much as possible to maximize compatibility.
I introduced the rename to gc.lib because that was what an existing FindGC.cmake was searching for. I'll have another close look at this with my current understanding. |
I have local changes to the CMakeLists.txt to produce binaries with the names I couldn't find a I also noticed that when building on Linux (using the steps on their README), you get Can I ask, how did you build the binaries before? |
The FindGC.cmake is checked in to the project I'm working on. In no way authoritative, just one example of what is out there... I never built GC before the approach I submitted here. |
/azp run |
I modified the CMakeLists.txt file to produce binaries with the |
Whenever you have time can you test the latest commit? I still haven't gotten |
Before I can try this out, I will first have to sort out some stuff in my local workspace. I will try over the weekend. To get UWP working, the most straightforward approach is to deactivate the build of 'cord' - with your changes that should simply mean to drop BDWGC_INSTALL_TOOLS from the portfile.cmake. As I understand it, cord is in no way essential for the libraries. It seems to be an interactive text editor using an advanced string class exploiting the garbage collector in some clever way. I doubt that anyone uses that code and I really see no reason to struggle with windows UI libraries when the goal is to get a low-level library compiled. |
Trying out the latest changes, I find that the include files are now installed as include/bdwgc/gc.h - why was that extra subdirectory introduced? The FindGC.cmake the the project that I am porting searches for include/gc.h which worked before, but now it breaks. Trivial to fix on either side, but it should be clarified what the proper way is. |
I'm modeling the port after the distributed Debian package, so the correct thing to do would be to remove the |
Submitted PR upstream (ivmai/bdwgc#281). |
/azp run |
As it stands, the current package works for all supported triplets. I'm inclined to merging it right now, and update the port once the patches have been merged upstream. @NNemec have you had time to test whether this port works for your project? |
My original submission works fine, I have not tried out any of the later versions and won't have a chance to do so for the next two weeks. Go ahead with merging. If I encounter any problems, it can still be fixed... |
After having finished this new port, I found that there were already two issues requesting bdwgc pointing to existing branches on other repos (#5471 and #6052). Feel free to pick either one of those or this one.