Skip to content
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

cmake apple framework support #1319

Merged

Conversation

geertbleyen
Copy link
Contributor

@geertbleyen geertbleyen commented Feb 29, 2024

Description

Add cmake option TBB_BUILD_APPLE_FRAMEWORKS to build oneTBB as Apple Frameworks.
This is needed to be able to create iOS compatible app bundles which need to embed TBB as framework, as using dylibs is prohibited by Apple.

Fixes #1252

  • - git commit message contains an appropriate signed-off-by string (see CONTRIBUTING.md for details)

Type of change

  • bug fix - change that fixes an issue
  • new feature - change that adds functionality
  • tests - change in tests
  • infrastructure - change in infrastructure and CI
  • documentation - documentation update

Tests

  • added - required for new features and some bug fixes
  • not needed

Documentation

  • updated in this PR
  • needs to be updated
  • not needed

Breaks backward compatibility

  • Yes
  • No
  • Unknown

Notify the following users

@isaevil

Other information

I've also ran into various integer type mismatches when building (mainly long <-> int) when I was working on this (using a M2 MacBook Pro). Not sure if I should include these fixes (just static_cast'ing to the expected types) in this PR or not?

@geertbleyen
Copy link
Contributor Author

geertbleyen commented Feb 29, 2024

Oopsie, will sort out the copyright CI failures first thing tomorrow.

@geertbleyen
Copy link
Contributor Author

Missing copyright updates in the edited files is fixed now.

@omalyshe omalyshe requested a review from aepanchi March 1, 2024 09:49
@geertbleyen
Copy link
Contributor Author

geertbleyen commented Mar 1, 2024

It appears VS code auto-formatted utils_dynamic_libs.h and broke it :(.
Pushed a new commit to rollback those unintended changes.

cmake/README.md Outdated
@@ -19,6 +19,7 @@ TBB_INSTALL_VARS:BOOL - Enable auto-generated vars installation(packages generat
TBB_VALGRIND_MEMCHECK:BOOL - Enable scan for memory leaks using Valgrind (OFF by default)
TBB_DISABLE_HWLOC_AUTOMATIC_SEARCH - Disable HWLOC automatic search by pkg-config tool (OFF by default)
TBB_ENABLE_IPO - Enable Interprocedural Optimization (IPO) during the compilation (ON by default)
TBB_BUILD_APPLE_FRAMEWORKS - Enable building Apple .frameworks instead of dylibs, only available on Apple platform. (OFF by default)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
TBB_BUILD_APPLE_FRAMEWORKS - Enable building Apple .frameworks instead of dylibs, only available on Apple platform. (OFF by default)
TBB_BUILD_APPLE_FRAMEWORKS - Enable the Apple* frameworks instead of dylibs, only available on the Apple platform. (OFF by default)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aepanchi I'm not sure why Apple needs a asterisk. Is it to mark it as copyright? If so, shouldn't the 2nd Apple in this string also have an asterisk?
Tbh, I'm also not 100% about the 'the' in front of Apple in this sentence. But then again, I'm not a native English speaker :).

Copy link
Contributor

Choose a reason for hiding this comment

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

@geertbleyen Yes, it is for the copyright purposes. Just one for the first mention in the file is enough

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, fixed.

@@ -46,9 +46,17 @@ namespace utils {
#endif
#define EXT ".dll"
#else
#if TBB_USE_APPLE_FRAMEWORKS
#define PREFIX // When build as Apple Framework, the binary has no lib prefix
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#define PREFIX // When build as Apple Framework, the binary has no lib prefix
#define PREFIX // When built as Apple* Framework, the binary has no lib prefix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

#if __APPLE__
#if TBB_USE_APPLE_FRAMEWORKS
#define EXT // When build as Apple Framework, the binary has no lib prefix
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#define EXT // When build as Apple Framework, the binary has no lib prefix
#define EXT // When built as Apple* Framework, the binary has no lib prefix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@geertbleyen
Copy link
Contributor Author

@isaevil Is anything else required from me to get this PR completed?
I'm waiting for this PR to complete to also update the onetbb conan recipe on conan center index to make this option for Apple frameworks available to Conan users.

@geertbleyen
Copy link
Contributor Author

Hi @isaevil , any progress on this PR?
Any more info or effort required here?

Copy link
Contributor

@isaevil isaevil left a comment

Choose a reason for hiding this comment

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

Hi, sorry for late review. Generally, it looks good, left some minor comment.

src/tbb/CMakeLists.txt Outdated Show resolved Hide resolved
cmake/vars_utils.cmake Outdated Show resolved Hide resolved
Add * to Apple to indicate trademark

Co-authored-by: Ilya Isaev <[email protected]>
Copy link
Contributor

@isaevil isaevil left a comment

Choose a reason for hiding this comment

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

LGTM

@isaevil isaevil merged commit 39b4ff4 into uxlfoundation:master Apr 24, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

building an iOS XCFramework for oneTBB
4 participants