-
Notifications
You must be signed in to change notification settings - Fork 56
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
feat: Add CMake support to C bindings #247
Conversation
c11e5b6
to
6994b4d
Compare
The Have you verified that it's actually possible to use CMake to build the Windows example from a release package?It doesn't look to me like the existing CI has been modified to include the top-level CMakeLists.txt file, and corrosion if that's required, in the release package. I'm also not sure we want to require the same top-level CMake file to build examples out of the release package that we use to build the bindings themselves. That certainly doesn't bring us much closer to providing a convenient way to use the bindings with CMake in real projects. But I don't know what the right solution is. |
It's still not possible to build for ARM Windows. Having example programs rely on the top-level CMakeLists.txt seems to be a common pattern. There may be a cleaner way to do this but I haven't found it. |
I'm surprised CMake doesn't have a variable that directly exposes the target architecture. While I appreciate your work on this, I don't see any clear benefit to this PR. Have any current or potential users of the C bindings tested this and found it helpful? |
On further consideration, it looks to me like this PR isn't actually a step backward in any way. After all, the current build.bat for the Windows example doesn't correctly handle ARM64 either. @DataTriny Can you create a test release package for this PR, if you did before? Then, if I can build and run the Windows example from that package, I'll merge this. Thanks. |
@mwcampbell I've made what I think is good progress on this. I've added missing macOS and Linux support to the build process and the Windows example now depends on AccessKit exactly like an external project would. One notable difference with CMake is that it considers accesskit.dll.lib as part of static linking hence this file will now end up inside the static folder. |
46bf409
to
a5d239d
Compare
@mwcampbell Due to technical issues I wasn't able to test the ARM64 build on real hardware, but I checked that compilation and linking were done properly, so there is little chance it wouldn't work. I haven't documented the possibility though, you just use the standard -A flag when configuring to target ARM64. I have little hope for macOS as it seems cross-compilation might be slightly different over there. I still have my SDL example sitting around, I'll submit it here after this PR is merged so we can test. And finally I think dynamic linking with the current CMake setup would be unpractical, but I don't know how to improve it at the moment... What do you think? |
I don't really care about dynamic linking for actual C or C++ projects. To me, the DLL is primarily useful for other languages that can access a C API via their FFI. I have a Windows ARM laptop sitting under my desk, but I haven't booted it in years. I'll give it a try in the next few days. |
One more thing on Windows DLLs though: I think we're missing |
Here's my feedback based on the test release: In the Windows MSVC directories, the import library for the DLL (the file currently called accesskit.dll.lib) should be in the shared directory, not the static directory, and it should be renamed to accesskit.lib, since that is easier to use with some build systems. The only reason why the Rust toolchain calls it accesskit.dll.lib is that it's in the same directory as the static library; import libraries for DLLs usually just have a .lib extension, not .dll.lib. If this makes it harder to support dynamic linking with CMake, then I think we can leave out that feature for now; I think it's more important to get the layout and naming convention right. The libraries that are required by AccessKit (bcrypt, UIAutomationCore, etc.) should be specified in the shared AccessKit CMake configuration, not in individual projects like the Windows example. We want to encapsulate the common parts of building a project with AccessKit as much as possible. And of course, platforms other than Windows will have different required libraries, so this will be particularly important for cross-platform projects like the SDL example. As far as I can tell, the DLL on Windows is exporting all the right symbols. The Also, if you're still feeling lost when it comes to CMake, I'd like to recommend a book on this subject: Professional CMake. It's available as a PDF, which as I recall was more or less accessible. |
I now honestly think that we have everything, from a user perspective at least. Unless you have other concerns, we could merge this as is, or I can rewrite the CI/CD so we are sure everything works on all supported platforms. I will keep the PR in draft state as I would personally prefer to rewrite the CD before merging. |
If you'd rather rewrite the CI first, go ahead and do that. |
41cfd8d
to
448336a
Compare
There is only one remaining issue: I can't build the Windows example from a release package:
These two symbols apparently come from Ws2_32.lib, which is in the list of linked libraries added to the target I created in accesskit-config.cmake. The libraries I build on my machine don't have this issue. I did not find a way to directly link the Windows libraries into accesskit.lib because Corrosion generates IMPORTED INTERFACE libraries for which I could not get target_link_libraries working... |
Those functions certainly aren't defined in ws2_32.dll; they're defined in ntdll.dll. So you'll need to add ntdll.lib to the list of system libraries that AccessKit requires on Windows. This became a requirement in Rust 1.69 or 1.70; I'm not sure which. |
448336a
to
2444836
Compare
Oh, good catch! I wasn't using latest Rust version locally, and I didn't expect the Win32 libs requirements to change often... Now it builds. Have you been able to validate the ARM64 Windows build? Unless you have other objections, I consider this ready now. |
BTW the CI failure is expected, since the headers job is not pointing to the latest version. |
I see just one possible problem with the layout of the release package: under lib/windows/ARCH/mingw/shared, I think the import library should still be called libaccesskit.a, not accesskit.lib. But if you had a specific reason for calling it accesskit.lib even for MinGW, that's fine. I never got around to getting my old Windows ARM laptop running again. I haven't booted it in over 4 years, and I'd have to search for the power adapter. I've asked on Mastodon if anyone with an ARM-based Windows machine can help. |
OK, a volunteer validated the Windows ARM64 build of the example for us. So as soon as the comment about the MinGW import library is addressed, we're ready to merge. |
Copy/paste mistake on my part I guess. It wouldn't have worked anyway. I think macOS dylib name selection was buggy as well as macOS is an Unix as far as CMake is concerned. |
I can have the cross-platform SDL example ready in a very short time, so we could delay releasing the next version if that's OK with you. |
I see no reason to rush this. Releasing CMake support along with the SDL example makes sense to me. |
@DataTriny Do you see any reason not to merge this PR now? The only reason I didn't merge it yesterday was that I didn't realize that the SDL example would be a separate PR. |
@mwcampbell Well, as you can see I had to make small changes here because I discovered missing libs when linking the SDL example on Linux. Maybe there are similar missing pieces on macOS that I can't find myself. I'd like, if possible, that we validate the SDL example on macOS first so we are confident that the libraries actually work. |
I can still see the full path to the library inside macOS dylib, so I'll try to add a Cargo config file later to fix that. |
bffdabb
to
6b932a9
Compare
- Add back macOS and Linux support - Remove Corrosion from tree since it's only an implementation detail - Decouple the Windows example from the top-level CMake project - Add ARM64 support (at least for Windows)
…selecting dylib name
6b932a9
to
fe2dcc6
Compare
Based on my analysis of the raw libaccesskit.dylib file, I think it is now relocatable. |
With the accesskit-config.cmake fixes in my branch, the SDL example builds and runs on macOS. |
Thanks @mwcampbell , then I guess this PR is complete and can be merged now! |
Co-authored-by: Matt Campbell <[email protected]>
Missing pieces: