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

Fix WatchOS simulator not supporting thread_local #5341

Merged

Conversation

dianaafanador3
Copy link
Contributor

@dianaafanador3 dianaafanador3 commented Mar 18, 2022

What, How & Why?

Fix an error when compiling a watchOS Simulator target on an M1 is not supporting Thread-local storage realm/realm-swift#7694
realm/realm-swift#7695

☑️ ToDos

  • 📝 Changelog update
  • 🚦 Tests (or not relevant)

@cla-bot cla-bot bot added the cla: yes label Mar 18, 2022
@dianaafanador3 dianaafanador3 force-pushed the dp/watchos_simulator_old_architecture_target_not_thread_local branch 3 times, most recently from 14ef21a to 2fdb31f Compare June 6, 2022 19:19
@dianaafanador3 dianaafanador3 changed the title Dp/watchos simulator old architecture target not thread local Fix WatchOS simulator not supporting thread_local Jun 6, 2022
@dianaafanador3 dianaafanador3 requested a review from tgoyne June 6, 2022 19:59
@dianaafanador3 dianaafanador3 marked this pull request as ready for review June 6, 2022 19:59
@@ -175,11 +175,11 @@ bool SimulatedFailure::do_check_trigger(FailureType failure_type) noexcept
return false;
}

#if (REALM_ARCHITECTURE_X86_32 && REALM_IOS) || (REALM_ARCHITECTURE_X86_64 && REALM_WATCHOS)
#if (REALM_ARCHITECTURE_X86_32 && REALM_IOS) || (REALM_ARCHITECTURE_X86_32 && REALM_WATCHOS) || (REALM_ARCHITECTURE_X86_64 && REALM_WATCHOS) || (REALM_ARCHITECTURE_ARM_64 && REALM_WATCHOS)
Copy link
Member

Choose a reason for hiding this comment

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

This is a really confusing condition. Is it just (REALM_WATCHOS && !REALM_ARCHITECTURE_ARM64_32) or is there something else going on?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We previously added a condition to support old intel simulators REALM_ARCHITECTURE_X86_32 running on 32 bits.
On this PR I'm adding a condition for the REALM_WATCHOS and architecture REALM_ARCHITECTURE_ARM_64 for watchOS simulators on M1 machines and REALM_WATCHOS combined with REALM_ARCHITECTURE_X86_64 to support Intel simulators running on 64 bits, after some user reported the same error (thread_local not supported).
I tried to find a way to define a variable that let us know if thread_local is available which will make easier the condition here, but didn't find anything.
This branch is tested by several user having this problem, issues are linked on the description of the pull request.
I'm open to suggestion how we can make this easier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tgoyne do you think we can review this, or you still having doubts about the added conditions?

@Vortec4800
Copy link

Just bumping this PR to get it some attention again, my project won't build without this fix and it would be great to have it merged so I can update Realm. It seems like we just need to solidify how this condition should be written, otherwise it's a pretty straightforward issue and fix.

@tgoyne tgoyne force-pushed the dp/watchos_simulator_old_architecture_target_not_thread_local branch from 2fdb31f to 58e9827 Compare July 6, 2022 03:28
@tgoyne tgoyne force-pushed the dp/watchos_simulator_old_architecture_target_not_thread_local branch from 58e9827 to 9c64f57 Compare July 6, 2022 03:29
@tgoyne tgoyne merged commit a1907d6 into master Jul 6, 2022
@tgoyne tgoyne deleted the dp/watchos_simulator_old_architecture_target_not_thread_local branch July 6, 2022 04:34
@dianaafanador3
Copy link
Contributor Author

Thanks for the fix @tgoyne

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants