Git Product home page Git Product logo

Comments (31)

pepicrft avatar pepicrft commented on June 24, 2024 1

I debugged a few projects with TCA, and noticed that the crashes happen when the Pointfree packages are linked statically, the default one that Tuist sets to packages integrated via Tuist. If you set the product to be dynamic (i.e., .framework), then it works. Here's the code snippet:

#if TUIST
import ProjectDescription

let packageSettings = PackageSettings(
    productTypes: [
            "ComposableArchitecture": .framework,
            "Dependencies": .framework,
            "Clocks": .framework,
            "ConcurrencyExtras": .framework,
            "CombineSchedulers": .framework,
            "IdentifiedCollections": .framework,
            "OrderedCollections": .framework,
            "_CollectionsUtilities": .framework,
            "DependenciesMacros": .framework,
            "SwiftUINavigationCore": .framework,
            "Perception": .framework,
            "CasePaths": .framework,
            "CustomDump": .framework,
            "XCTestDynamicOverlay": .framework
        ]
)
#endif

I was quite surprised that the packages wouldn't work with static linking, but then I checked how SPM integrates the packages into projects, and they do it dynamically too, something that makes me think that the packages might be strongly coupled to the dynamic linking through their usage of APIs.

from tuist.

mbrandonw avatar mbrandonw commented on June 24, 2024 1

Hi @pepicrft, understood and thanks for clarifying. Your edits to the blog post paint the picture more clearly now, thanks for updating.

from tuist.

mbrandonw avatar mbrandonw commented on June 24, 2024

I've asked Brandon for help, but he suggested to seek for help here. He claims this is not the first time people are having issues with marrying Tuist and TCA, so I hope someone here may have the expertise.

Just to clarify a bit, this code does not crash when run as a regular Xcode or SPM project. It seems to only happen when run in a Tuist project.

from tuist.

iharandreyev avatar iharandreyev commented on June 24, 2024

@mbrandonw Correct. I've created a project manually and pushed it into the sample repo. It works as expected. But when running the same code from twist-generated project, the aforementioned crash occurs

from tuist.

iharandreyev avatar iharandreyev commented on June 24, 2024

One more note, the crash occurs only on iOS 17.5 (and I assume anything 17.0 and above), but not 15.5 (an I assume anything below 17.0, as mentioned in the TCA issue discussion)

from tuist.

pepicrft avatar pepicrft commented on June 24, 2024

Thanks for reporting this, @iharandreyev, and thanks, @mbrandonw, for chiming in.

I marked this as a high priority, and we'll look into it next week. For context, we convert Swift Packages into Xcode native targets with build settings and phases. The standard integration of Swift Package Manager into Xcode sometimes infers some build and linking configurations that are resolved at build-time to make the integration work, and we have to look into those settings to make sure we use the same ones in our integration.

When these issues happen, we usually debug them by:

  • Integrating the project through Xcode's standard integration and looking at the compiler flag that's used with the file whose compilation is failing.
  • Integrating it via Tuist using native Xcode targets and seeing the compilation flag that's being used with the same file.
  • Compare the two and see where the differences are.

It'd be awesome if you could help us with that part. Otherwise, we'll do that ourselves next week. Apologies @iharandreyev for the invonveniences.

from tuist.

iharandreyev avatar iharandreyev commented on June 24, 2024

The first thing I notice is that for manually created project Xcode lists dependencies explicitly, while for tuist generated project - implicitly.
Don't know how it's supposed to be.
The logs are enormous, so digging deeper will take some time

Build Tuist-TCA-LinkingIssue_2024-05-24T11-55-27.txt

Build Tuist-TCA-LinkingIssue_Created_Manually_2024-05-24T11-56-08.txt

from tuist.

kapitoshka438 avatar kapitoshka438 commented on June 24, 2024

@iharandreyev I don't know if these experiments can help but I've run tests for two versions of your sample project:

Regular XCode+SPM project:

Test Suite 'LinkingIssueSampleAppTests' started at 2024-05-24 11:29:59.804.
Test Case '-[TCAIssueTests.LinkingIssueSampleAppTests testReducer]' started.
/Users/EMiniakhmetov/Documents/TCAIssue/TCAIssueTests/TCAIssueTests.swift:14: error: -[TCAIssueTests.LinkingIssueSampleAppTests testReducer] : Expected state to change, but no change occurred.

The trailing closure made no observable modifications to state. If no change to state is expected, omit the trailing closure.
Test Case '-[TCAIssueTests.LinkingIssueSampleAppTests testReducer]' failed (0.660 seconds).
Test Suite 'LinkingIssueSampleAppTests' failed at 2024-05-24 11:30:00.464.
	 Executed 1 test, with 1 failure (0 unexpected) in 0.660 (0.660) seconds

Tuist generated project:

objc[42120]: Class _TtC22ComposableArchitecture6Logger is implemented in both /Users/EMiniakhmetov/Library/Developer/CoreSimulator/Devices/614101E6-FC56-4123-BA43-1E5368DC06B6/data/Containers/Bundle/Application/556C4807-55D9-4FF7-8DCE-8E73BBEF47A6/Tuist_TCA_LinkingIssue.app/Tuist_TCA_LinkingIssue (0x102941da0) and /Users/EMiniakhmetov/Library/Developer/CoreSimulator/Devices/614101E6-FC56-4123-BA43-1E5368DC06B6/data/Containers/Bundle/Application/556C4807-55D9-4FF7-8DCE-8E73BBEF47A6/Tuist_TCA_LinkingIssue.app/PlugIns/Tuist_TCA_LinkingIssueTests.xctest/Tuist_TCA_LinkingIssueTests (0x106a8cc80). One of the two will be used. Which one is undefined.
Test Suite 'All tests' started at 2024-05-24 11:39:43.137.
Test Suite 'Tuist_TCA_LinkingIssueTests.xctest' started at 2024-05-24 11:39:43.137.
Test Suite 'LinkingIssueSampleAppTests' started at 2024-05-24 11:39:43.138.
Test Case '-[Tuist_TCA_LinkingIssueTests.LinkingIssueSampleAppTests testReducer]' started.
Could not cast value of type 'Observation.ObservationRegistrar' (0x210440b10) to 'Perception._PerceptionRegistrar' (0x10293dfd8).
Снимок экрана 2024-05-24 в 11 40 11

Then I added "-ObjC" flag to OTHER_LDFLAGS to the main target build settings for the Tuist generated project and the result became similar to the regular project:
Снимок экрана 2024-05-24 в 11 43 35

Test Suite 'All tests' started at 2024-05-24 11:41:36.561.
Test Suite 'Tuist_TCA_LinkingIssueTests.xctest' started at 2024-05-24 11:41:36.561.
Test Suite 'LinkingIssueSampleAppTests' started at 2024-05-24 11:41:36.562.
Test Case '-[Tuist_TCA_LinkingIssueTests.LinkingIssueSampleAppTests testReducer]' started.
/Users/EMiniakhmetov/Downloads/Tuist-CTA-LinkingIssue-main/Tuist-TCA-LinkingIssue/Tests/Tuist-TCA-LinkingIssueTests.swift:14: error: -[Tuist_TCA_LinkingIssueTests.LinkingIssueSampleAppTests testReducer] : Expected state to change, but no change occurred.

The trailing closure made no observable modifications to state. If no change to state is expected, omit the trailing closure.
Test Case '-[Tuist_TCA_LinkingIssueTests.LinkingIssueSampleAppTests testReducer]' failed (0.044 seconds).
Test Suite 'LinkingIssueSampleAppTests' failed at 2024-05-24 11:41:36.606.
	 Executed 1 test, with 1 failure (0 unexpected) in 0.044 (0.044) seconds
Test Suite 'Tuist_TCA_LinkingIssueTests.xctest' failed at 2024-05-24 11:41:36.606.
	 Executed 1 test, with 1 failure (0 unexpected) in 0.044 (0.045) seconds
Test Suite 'All tests' failed at 2024-05-24 11:41:36.606.
	 Executed 1 test, with 1 failure (0 unexpected) in 0.044 (0.045) seconds

from tuist.

pepicrft avatar pepicrft commented on June 24, 2024

@hisaac and @thedavidharris added some documentation recently explaining some additional configuration that might required for packages that have Objective-C code.
Note that dependency managers like Swift Package Manager, might try to make things work at build-time. We tried to do the same too by applying those flags automatically, but as @thedavidharris pointed out, that's not a good idea because it can cause an increase in the binary size or duplicated symbols.

from tuist.

pepicrft avatar pepicrft commented on June 24, 2024

@iharandreyev do you mind trying the change suggested by @kapitoshka438 and reporting back?

from tuist.

Narsail avatar Narsail commented on June 24, 2024

@pepicrft I'm curious about the design decision to deviate from what swift package manager is doing (including -ObjC and risk possible side effects)?

For me, the missing -ObjC resulted in a 3 day long venture into testing multiple different setups to modularise my codebase with tuist. I'd rather put it in as a default (something iOS developers are used to working with swift packager manager) and remove it when it causes side-effects.

Going a different way than the package manager might cause onboarding friction to tuist (and has for me).

^ Can someone elaborate on the possible side effects that including -ObjC has? The documentation wasn't clear on that point.

from tuist.

fortmarek avatar fortmarek commented on June 24, 2024

@Narsail

I'm curious about the design decision to deviate from what swift package manager is doing (including -ObjC and risk possible side effects)?

@thedavidharris went into this here. Applying -ObjC flag whenever a target has ObjC sources is not the best path forward. We'd love to include when it's necessary but there's not really a good way to do that.

Going a different way than the package manager might cause onboarding friction to tuist (and has for me).

We're trying to stay as close to SPM as possible. But applying -ObjC whenver a target has objc sources is actually not what SPM does. In fact, it doesn't seem SPM applies the -ObjC flag directly at all. We haven't been able to figure out why SPM integration doesn't need the linker flag in some cases whereas we do. Ideally, we'd investigate further and see if we can apply some default that would actually align with SPM.

from tuist.

Narsail avatar Narsail commented on June 24, 2024

Thank you. That was helpful 🙏

from tuist.

iharandreyev avatar iharandreyev commented on June 24, 2024

@iharandreyev do you mind trying the change suggested by @kapitoshka438 and reporting back?

I've tried the method, and it did work. What worries me is that I don't understand why (mostly since I am not knowledgeable about the ways of Xcode linking and building things)

I've also tried setting the flag manually to individual dependency targets (swift-perception and swift-composable-architecture), but no luck.

@pepicrft Are we still interested in contents of Xcode logs? I've started building logs diffing tool, but was not able to finish it. And don't know if there's any necessity in it anymore.

from tuist.

iharandreyev avatar iharandreyev commented on June 24, 2024

I debugged a few projects with TCA, and noticed that the crashes happen when the Pointfree packages are linked statically, the default one that Tuist sets to packages integrated via Tuist. If you set the product to be dynamic (i.e., .framework), then it works. Here's the code snippet:

#if TUIST
import ProjectDescription

let packageSettings = PackageSettings(
    productTypes: [
            "ComposableArchitecture": .framework,
            "Dependencies": .framework,
            "Clocks": .framework,
            "ConcurrencyExtras": .framework,
            "CombineSchedulers": .framework,
            "IdentifiedCollections": .framework,
            "OrderedCollections": .framework,
            "_CollectionsUtilities": .framework,
            "DependenciesMacros": .framework,
            "SwiftUINavigationCore": .framework,
            "Perception": .framework,
            "CasePaths": .framework,
            "CustomDump": .framework,
            "XCTestDynamicOverlay": .framework
        ]
)
#endif

I was quite surprised that the packages wouldn't work with static linking, but then I checked how SPM integrates the packages into projects, and they do it dynamically too, something that makes me think that the packages might be strongly coupled to the dynamic linking through their usage of APIs.

It turns out that for this specific case only a subset of dependencies should be dynamic:

let packageSettings = PackageSettings(
    productTypes: [
        "ComposableArchitecture": .framework,
        "Perception": .framework
    ]
)

results in crash-free test.
However tuist generate outputs a warning

The following warnings need attention:
 · Target 'XCTestDynamicOverlay' has been linked from target 'ComposableArchitecture' and target 'Perception', it is a static product so may introduce unwanted side effects.

Adding this dependency as dynamic framework

let packageSettings = PackageSettings(
    productTypes: [
        "ComposableArchitecture": .framework,
        "Perception": .framework,
        "XCTestDynamicOverlay": .framework
    ]
)

results in to tuist warnings an a crash-free test execution.

from tuist.

pepicrft avatar pepicrft commented on June 24, 2024

Thanks for the update @iharandreyev.
I opened this PR that adds some documentation around linking and some configuration that might be required by packages for things to work at runtime.

from tuist.

thedavidharris avatar thedavidharris commented on June 24, 2024

Sorry, just seeing this. This feels odd... -ObjC shouldn't have any impact on pure Swift code... but it appears it does https://github.com/apple-opensource/ld64/blob/e28c028b20af187a16a7161d89e91868a450cadc/src/ld/parsers/macho_relocatable_file.cpp#L1520~L1525

Wonder if something with _PerceptionRegistratrar is not getting fully linked.

Some linked relevant threads:
https://forums.swift.org/t/linker-flag-objc-force-loads-swift-libraries/47466
apple/swift#48561

Actually seems very similar to https://github.com/airbnb/BuckSample/pull/38/files. @mbrandonw does that track?

from tuist.

kapitoshka438 avatar kapitoshka438 commented on June 24, 2024

@thedavidharris TCA is not really a pure swift:
https://github.com/pointfreeco/swift-composable-architecture/blob/7cbde3b07f193f732e256429e1351ff53cd31641/Sources/ComposableArchitecture/UIKit/NSObject%2BObservation.swift#L172
The implementation of the method above uses Objective C categories which are the main trigger for turning on the -ObjC flag.

from tuist.

pepicrft avatar pepicrft commented on June 24, 2024

@thedavidharris TCA is not really a pure swift: https://github.com/pointfreeco/swift-composable-architecture/blob/7cbde3b07f193f732e256429e1351ff53cd31641/Sources/ComposableArchitecture/UIKit/NSObject%2BObservation.swift#L172 The implementation of the method above uses Objective C categories which are the main trigger for turning on the -ObjC flag.

I wonder if something can be done at the package level, maybe referencing those symbols statically, so that the dead-stripping logic keeps the symbols and prevents developers from having to use the -ObjC build setting.
I also wonder how SPM does when it integrates packages? Because as far as I know, there are no issues when integrating composable packages through SPM's standard integration.

from tuist.

shahzadmajeed avatar shahzadmajeed commented on June 24, 2024

Maybe SPM doesn't have optimizations and it links everything by default? Xcode does a lot of magic internally which I wish was documented anywhere

from tuist.

thedavidharris avatar thedavidharris commented on June 24, 2024

@thedavidharris TCA is not really a pure swift: https://github.com/pointfreeco/swift-composable-architecture/blob/7cbde3b07f193f732e256429e1351ff53cd31641/Sources/ComposableArchitecture/UIKit/NSObject%2BObservation.swift#L172 The implementation of the method above uses Objective C categories which are the main trigger for turning on the -ObjC flag.

Thanks, that makes sense. We're on a version of TCA prior to this at the moment while we migrate, so haven't run into it.

In theory I think it's better to -force-load some of those specific items.

Anyway, asking https://forums.swift.org/t/linker-flag-objc-force-loads-swift-libraries/47466/7 to see if anyone knows the implementation detail. I'm guessing that someone SPM itself does some force-linking, which causes things like https://forums.swift.org/t/objc-flag-causes-duplicate-symbols-with-swift-packages/27926.

from tuist.

mbrandonw avatar mbrandonw commented on June 24, 2024

There is some interesting information in this Swift issue I filed awhile back. If there is something we can do in the library to help we will certainly consider making changes, but the fact that it works fine in SPM makes us hesitant.

from tuist.

thedavidharris avatar thedavidharris commented on June 24, 2024

Looks right. I assume Bazel is getting around this with https://github.com/bazel-ios/rules_ios/blob/master/rules/force_load_direct_deps.bzl.

from tuist.

mbrandonw avatar mbrandonw commented on June 24, 2024

I wonder if something can be done at the package level, maybe referencing those symbols statically, so that the dead-stripping logic keeps the symbols and prevents developers from having to use the -ObjC build setting.

Hi @pepicrft, can you explain this in detail a bit more? What symbols are you referring to? The actual crash experienced is on this line:

PNG image

…and the reason we get to this line is because the following if fails:

if
  #available(iOS 17, macOS 14, tvOS 17, watchOS 10, *),
  let subject = subject as? any Observable
{

…and this if fails because somehow the subject is not an Observable, even though subject in this case is a Store, which is definitely Observable.

And none of this code is touching the little bit of Objective-C code that was linked to previously in the discussion. In fact, I can completely delete all of that Objective-C code and the exact same crash happens, and so that leads me to believe it is not related.

While I understand it can be quite difficult creating a build tool meant to replace Apple's opaque tooling, it has not been great for tuist's maintainer to blame 3rd party packages for problems when integrated with tuist, and further write blog posts specifically mentioning 3rd party packages that do not work with tuist.

If the maintainers have some concrete advice to provide (or even better, a PR to fix!), then we would happily take the time to look into it. But right now I'm not sure the core problem has actually been uncovered, as it seems to have nothing to do with Objective-C. And the other libraries that have seemingly fixed the issue (Promises, IGListKit) were dealing with Objective-C problems which do not seem to be at play here.

from tuist.

kapitoshka438 avatar kapitoshka438 commented on June 24, 2024

@mbrandonw for some reason I'm sure that if you move this peace of code from Store+Observation.swift to Store.swift the problem will go away:

#if canImport(Observation)
  @available(iOS 17, macOS 14, tvOS 17, watchOS 10, *)
  extension Store: Observable {}
#endif

from tuist.

kapitoshka438 avatar kapitoshka438 commented on June 24, 2024

I was able to fix the crash by doing what described in the previous comment.

And it really looks like a swift bug apple/swift#48561.
Another approach that fixes the issue is to enable the "Perform Single-Object Prelink" linking option (GENERATE_MASTER_OBJECT_FILE = YES)
apple/swift#48561 (comment)

from tuist.

mbrandonw avatar mbrandonw commented on June 24, 2024

Hi @kapitoshka438, yep can confirm, good catch! I'm happy to make the PR to TCA myself, but if you rather that is ok with us too. 🙂

from tuist.

kapitoshka438 avatar kapitoshka438 commented on June 24, 2024

Below are build log of similar sample projects:

  1. Native_SPM_build_log.txt
    This is a classic SPM way. You can see that linker commands Ld are called with the -r argument. This argument is exactly what the GENERATE_MASTER_OBJECT_FILE option is responsible for.
  2. Tuist_build_log_MasterObject.txt
    This is our issue where the test crashes. No -r argument for linking.
  3. Tuist_build_log.txt
    This is similar to the previous Tuist way but with GENERATE_MASTER_OBJECT_FILE enabled for the swift-composable-architecture project. -rs are presented in the build log. No crash.

Having that I conclude that the way how SPM does the builds without the "-ObjC" flag is found! ))

from tuist.

kapitoshka438 avatar kapitoshka438 commented on June 24, 2024

Hi @kapitoshka438, yep can confirm, good catch! I'm happy to make the PR to TCA myself, but if you rather that is ok with us too. 🙂

Thank you @mbrandonw I'd rather trust it to you. 🙂

from tuist.

thedavidharris avatar thedavidharris commented on June 24, 2024

Good find @kapitoshka438! And thanks @mbrandonw. This explains a bit of why alwayslink was necessary with Bazel too for these libraries if

As far as the general -ObjC thing goes, I've known of generally good ways to fix that for Objective-C libraries so that no consumption flags are needed (did not know about the prelink though, but apparently that's been known for 8 years!) but I have generally been stumped about this specific Swift one.

The same file case works well for this one, but I'm curious about the implications of apple/swift#65901 and https://forums.swift.org/t/pitch-low-level-linkage-control-attributes-used-and-section/65877 going forward.

It seems non-ideal for appsize that -r is applied, especially given the state of cross-module dead code stripping; I do wonder why Apple decided to make that the default, perhaps because adding any sort of -ObjC or force-load to OTHER_LDFLAGS is not allowed without unsafeFlags?

from tuist.

pepicrft avatar pepicrft commented on June 24, 2024

Excellent finding, @kapitoshka438 👏🏼, and thanks for the thorough explanation. Also thanks @mbrandonw for your support.

While I understand it can be quite difficult creating a build tool meant to replace Apple's opaque tooling, it has not been great for tuist's maintainer to blame 3rd party packages for problems when integrated with tuist, and further write blog posts specifically mentioning 3rd party packages that do not work with tuist.

@mbrandonw I did not intend to blame third-party packages in the blog post. If that's how it came across, I apologize. I extended that part to mention that this might only happen when they integrate with Tuist.

from tuist.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.