Skip to content

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

Closed
wants to merge 2 commits into from

Conversation

ngrewe
Copy link

@ngrewe ngrewe commented Nov 24, 2019

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 on BlocksRuntime_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_directories directive in the CMakeLists.txt for the tests/ subdirectory. The effect of that is already achieved by linking the BlocksRuntime::BlocksRuntime target and it started tripping up things once switching back to find_package() for the runtime lib.

Please let me know what you think!

Thanks,

Niels

This allows the user to provide a version of libBlocksRuntime on
systems other than Darwin.
@ngrewe
Copy link
Author

ngrewe commented Jan 4, 2020

Hey folks,

is there any feedback on this? – I'm happy to rework it as needed.

Thanks!

Niels

@triplef
Copy link
Contributor

triplef commented Feb 5, 2020

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?

@triplef
Copy link
Contributor

triplef commented May 12, 2020

@compnerd would you mind reviewing this? 🙏

@compnerd
Copy link
Member

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.

Copy link
Member

@compnerd compnerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A previous comment.

@shahmishal shahmishal closed this Oct 6, 2020
@shahmishal
Copy link
Member

The Swift project moved the default branch to main and deleted master branch, so GitHub automatically closed the PR. Please re-create the pull request with main branch.

More detail about the branch update - https://forums.swift.org/t/updating-branch-names/40412

triplef added a commit to gnustep/tools-windows-msvc that referenced this pull request Mar 19, 2021
triplef added a commit to gnustep/tools-android that referenced this pull request May 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants