-
Notifications
You must be signed in to change notification settings - Fork 480
build: allow system-supplied libBlocksRuntime on systems other than Darwin #534
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
Conversation
This allows the user to provide a version of libBlocksRuntime on systems other than Darwin.
|
Hey folks, is there any feedback on this? – I'm happy to rework it as needed. Thanks! Niels |
|
We’d also like to see this merged – it’d be great if the maintainers could provide feedback. 🙏 Was also wondering if #535 was related? |
|
@compnerd would you mind reviewing this? 🙏 |
|
The removal wasn’t really accidental. The reason is that there’s a number of different blocks runtimes and each has slightly different behaviors in edge cases. This ensures that all the platform are the same behavior. I’ll defer to Apple if they want to support different implications, but overall, I think that requiring this version is best. The rest of the swift build system currently relies on this, so if the external variant is desired to be supported, someone needs to clean up the swift build system first. |
compnerd
left a comment
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.
A previous comment.
|
The Swift project moved the default branch to More detail about the branch update - https://forums.swift.org/t/updating-branch-names/40412 |
In the past, the GNUstep project has used the ability so link (a fairly ancient version of) libdispatch against a custom libBlocksRuntime in order to ensure interoperability with its Objective-C runtime (which bundles a BlocksRuntime implementation). It seems that this facility was removed in PR #396 in favour of a check on whether it's building for Darwin to decide whether the in-tree version should be built.
That seems accidental, given the stated purpose of that PR, so I would very much like to re-add that facility. I have now done this by re-adding the elided
find_package(BlocksRuntime QUIET)statement and deciding whether to build the in-tree version depending onBlocksRuntime_FOUND(also the same as prior to PR #396). I hope this is what's intended here. I've checked that this works on a Linux system both with and without a custom libBlocksRuntime.Unfortunately, I couldn't convince libdispatch to build on my macOS machine – either with or without this change, so that is probably due to my lack of expertise. But it could very well be possible that building the in-tree libBlocksRuntime needs to be conditional on
NOT (CMAKE_SYSTEM_NAME STREQUAL Darwin OR BlocksRuntime_FOUND).Additionally, yet relatedly, there was an extraneous
target_include_directoriesdirective in the CMakeLists.txt for thetests/subdirectory. The effect of that is already achieved by linking theBlocksRuntime::BlocksRuntimetarget and it started tripping up things once switching back tofind_package()for the runtime lib.Please let me know what you think!
Thanks,
Niels