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

Cannot integrate Amplify 2.x.x dependency via SPM #5320

Closed
nonameplum opened this issue Jul 31, 2023 · 14 comments
Closed

Cannot integrate Amplify 2.x.x dependency via SPM #5320

nonameplum opened this issue Jul 31, 2023 · 14 comments
Labels
bounty Resolving this issue is rewarded with a bounty: https://github.com/tuist/tuist/discussions/4982 type:need/problem Report needs or problems you came across that are not bugs

Comments

@nonameplum
Copy link

What problem or need do you have?

I have exactly this issue aws-amplify/amplify-swift#2759.
I cannot build the project once I start depending on:

.external(name: "Amplify"),
.external(name: "AWSCognitoAuthPlugin"),
.external(name: "AWSAPIPlugin"),

in result I get compilation errors:

'../amplify_tommath.h' file not found
Could not build Objective-C module 'libtommathAmplify'

I tried to add header files in HEADER_SEARCH_PATHS but, so the original issue with '../amplify_tommath.h' file not found seems to be gone but then new ones occur from other AWS C'ish dependencies that finally the C99 compiler errors occur (e.g. unknown bool types etc).

I created smallest possible example project that represents the problem:
https://github.com/nonameplum/tuist-amplify

Potential solution

No response

macOS version

13.4.1

Tuist version

3.14.0

Xcode version

14.3.1

@nonameplum nonameplum added the type:need/problem Report needs or problems you came across that are not bugs label Jul 31, 2023
@hoangta
Copy link

hoangta commented Aug 2, 2023

I encounter this issue before & I had a look at your example , you can fix it as:
Change this from Dependencies.swift

let dependencies = Dependencies(
    swiftPackageManager: [
        .remote(url: "https://github.com/aws-amplify/amplify-swift", requirement: .upToNextMajor(from: "2.15.1")),
    ],
    platforms: [.iOS]
)

to

let spm = SwiftPackageManagerDependencies([
    .remote(url: "https://github.com/aws-amplify/amplify-swift", requirement: .upToNextMajor(from: "2.15.1"))
], targetSettings: [
    "AmplifyBigInteger": [
        "OTHER_SWIFT_FLAGS": "-Xcc -Wno-error=non-modular-include-in-framework-module",
        "HEADER_SEARCH_PATHS": .array([
            "$(inherited)",
            "$(SRCROOT)/AmplifyPlugins/Auth/Sources/libtommath/include"
        ])
    ]
])

let dependencies = Dependencies(
    swiftPackageManager: spm,
    platforms: [.iOS]
)

Change this from Project+Templates.swift, line 24

let sources = Target(name: name,
                platform: platform,
                product: .framework,
                bundleId: "io.tuist.\(name)",
                infoPlist: .default,
                sources: ["Targets/\(name)/Sources/**"],
                resources: [],
                dependencies: [
                    .external(name: "Amplify"),
                    .external(name: "AWSCognitoAuthPlugin"),
                    .external(name: "AWSAPIPlugin"),
                ])

to

let sources = Target(
            name: name,
            platform: platform,
            product: .framework,
            bundleId: "io.tuist.\(name)",
            infoPlist: .default,
            sources: ["Targets/\(name)/Sources/**"],
            resources: [],
            dependencies: [
                .external(name: "Amplify"),
                .external(name: "AWSCognitoAuthPlugin"),
                .external(name: "AWSAPIPlugin"),
            ],
            settings: .settings(base: [
                "OTHER_SWIFT_FLAGS": "-Xcc -Wno-error=non-modular-include-in-framework-module",
                "HEADER_SEARCH_PATHS": .array([
                    "$(inherited)",
                    "$(SRCROOT)/Tuist/Dependencies/SwiftPackageManager/.build/checkouts/aws-crt-swift/aws-common-runtime/aws-c-auth/include",
                    "$(SRCROOT)/Tuist/Dependencies/SwiftPackageManager/.build/checkouts/aws-crt-swift/aws-common-runtime/aws-c-cal/include",
                    "$(SRCROOT)/Tuist/Dependencies/SwiftPackageManager/.build/checkouts/aws-crt-swift/aws-common-runtime/aws-c-io/include",
                    "$(SRCROOT)/Tuist/Dependencies/SwiftPackageManager/.build/checkouts/aws-crt-swift/aws-common-runtime/aws-c-common/include",
                    "$(SRCROOT)/Tuist/Dependencies/SwiftPackageManager/.build/checkouts/aws-crt-swift/aws-common-runtime/aws-c-sdkutils/include",
                    "$(SRCROOT)/Tuist/Dependencies/SwiftPackageManager/.build/checkouts/aws-crt-swift/aws-common-runtime/aws-c-http/include",
                    "$(SRCROOT)/Tuist/Dependencies/SwiftPackageManager/.build/checkouts/aws-crt-swift/aws-common-runtime/aws-c-compression/include",
                    "$(SRCROOT)/Tuist/Dependencies/SwiftPackageManager/.build/checkouts/aws-crt-swift/aws-common-runtime/config",
                    "$(SRCROOT)/Tuist/Dependencies/SwiftPackageManager/.build/checkouts/aws-crt-swift/aws-common-runtime/aws-c-event-stream/include",
                    "$(SRCROOT)/Tuist/Dependencies/SwiftPackageManager/.build/checkouts/amplify-swift/AmplifyPlugins/Auth/Sources/libtommath/include"
                ])
            ])
        )

Generally, this fix can be referenced from https://docs.tuist.io/guides/third-party-dependencies/#some-notes-on-the-integration-of-swift-packages, but not only for your app targets (TuistAmplifyKit) but for 1 of the amplify target also (AmplifyBigInteger), and the non-modular header inside framework module issue

nonameplum added a commit to nonameplum/tuist-amplify that referenced this issue Sep 13, 2023
…orretly to build the project

There is another issue though. During runtime the Amplify is not configuring plugins because for some reason it doesn't see that it implements `CategoryConfigurable` protocol.
Which works fine when Amplify is integrated using legacy `packages` method - https://docs.tuist.io/1/manifests/project/#package
@nonameplum
Copy link
Author

nonameplum commented Sep 13, 2023

@hoangta Thank you very much for your tip. It truly works as the project builds.

There is another issue though. During runtime Amplify is not configuring plugins because for some reason it doesn't see that it implements CategoryConfigurable protocol.
Which works fine when Amplify is integrated using legacy packages method - https://docs.tuist.io/1/manifests/project/#package

I have created another branch with your proposed changes which fixed building but shows the issue during the runtime.

After digging deeper the issue is in the fragment of code in Amplify:

    private static func configure(_ candidate: Category, using configuration: AmplifyConfiguration) throws {
        guard let configurable = candidate as? CategoryConfigurable else {
            return
        }

        try configurable.configure(using: configuration)
    }

casting candidate to CategoryConfigurable doesn't work, beside of the fact that the protocol is implemented e.g. by APICategory https://github.com/aws-amplify/amplify-swift/blob/6f95db94d766921c92fef5631cb9b867899bdeca/Amplify/Categories/API/Internal/APICategory%2BCategoryConfigurable.swift#L8

If I use exactly the same code, but Amplify is integrated using legacy method (packages) it works correctly, and the casting to the CategoryConfigurable protocol works fine.

@fortmarek fortmarek added the bounty Resolving this issue is rewarded with a bounty: https://github.com/tuist/tuist/discussions/4982 label Oct 19, 2023
@stefanomondino
Copy link
Collaborator

Some more details I've discovered so far. I think the "objective-c" category stuff I've said earlier it's pointless, forget about it.

It seems like something between Tuist and Amplify is adding some kind of "hidden compilation flag" that strips away code contained in files that is not directly referenced by the code itself.

Amplify (in this case) is doing this thing:

  • they have a global protocol Category they use all around
  • they have a more specific protocol ConfigurableCategory with some additional methods
  • they implement conformance of some objects (defined as conforming to Category) to ConfigurableCategory in extensions that only have the conformance itself and nothing else. Example: extension PushNotificationsCategory: ConfigurableCategory
  • there's a method that take a Category object (with no direct reference to its concrete type - just passed around with protocol conformance) and inside that method they do what @nonameplum wrote before:
   private static func configure(_ candidate: Category, using configuration: AmplifyConfiguration) throws {
        guard let configurable = candidate as? CategoryConfigurable else {
            return
        }
        try configurable.configure(using: configuration)
    }
    
    //and then the method is called with something like:
    configure(PushNotificationCategory(), using: configuration)
  • by trying to cast the candidate the PushNotificationCategory object to CategoryConfigurable there's basically no direct call that links the object to the methods that are declared in the extension. Therefore, since nobody is (apparently) referencing that extension (actually, that FILE) at compile time, seems like the file contents are not compiled.
    In other words, commenting out all contents of that file doesn't affect at all the whole compilation - the program compiles anyway.
    But if I add some kind of dummy object in that file and reference it somewhere, it works.

It's really hard to explain in words, and it's also hard to push some kind of demo repo about it because I'm directly changing the AWS source code that gets cleared up everytime you fetch it. I've tried to reproduce this in a more simple project but it works as expected.

There's definetly something going on with a target with objc / C files, that custom flags we are using (-Xcc -Wno-error=non-modular-include-in-framework-module") and who knows what else.

@danieleformichelli
Copy link
Collaborator

I remember some bug about static x frameworks which resembles this. Can you see I setting the framework as dynamic fixes the problem?

@stefanomondino
Copy link
Collaborator

@danyf90 I'm seriously impressed by your memory. Changing (manually) to dynamic library actually seems to fix it (I'm going to dig into it and find out more).

Do you happen to remember where you first saw that bug? I mean, was it an apple rdar or something?

@stefanomondino
Copy link
Collaborator

https://forums.swift.org/t/extension-only-compilation-unit-in-static-framework/15241 got it

@stefanomondino
Copy link
Collaborator

ok so I think it can be fixed by setting GENERATE_MASTER_OBJECT_FILE to YES (a.k.a. Perform Single-Object Prelink).

So basically, in dependencies and summing everything up, something like this:

let spm = SwiftPackageManagerDependencies([
    .remote(url: "https://github.com/aws-amplify/amplify-swift", requirement: .upToNextMajor(from: "2.15.1"))
], targetSettings: [
    "Amplify": [
       "GENERATE_MASTER_OBJECT_FILE": "YES"
     ],
    "AmplifyBigInteger": [
        "OTHER_SWIFT_FLAGS": "-Xcc -Wno-error=non-modular-include-in-framework-module",
        "HEADER_SEARCH_PATHS": .array([
            "$(inherited)",
            "$(SRCROOT)/AmplifyPlugins/Auth/Sources/libtommath/include"
        ])
    ]
])

@danieleformichelli
Copy link
Collaborator

Thanks @stefanomondino !

Would you mind adding this custom mapping for Amplify to Tuist, so other users can just use it out of the box?

You just need to add it here:

// Hardcoded mapping for some well known libraries, until the logic can handle those properly

Instead, the -Xcc -Wno-error=non-modular-include-in-framework-module I think can be added i any case here:

@stefanomondino
Copy link
Collaborator

@danyf90 I'd love to do that! Let me work on that (hopefully) tomorrow and I'll draft a PR. Unfortunatly I also think there's some work involved directly on the target (not only on the dependencies/package manager side) as previously stated in this thread, otherwise Amplify won't compile. (#5320 (comment))

And settings required in there are strictly related to the current root where the project is running, so I don't think it's something "standardizable".

But I'll start with those mappings for SPM.

nonameplum added a commit to nonameplum/tuist-amplify that referenced this issue Oct 20, 2023
@nonameplum
Copy link
Author

nonameplum commented Oct 20, 2023

@stefanomondino @danyf90 Great work guys. I can confirm that by adding the setting "GENERATE_MASTER_OBJECT_FILE": "YES" the runtime issue is fixed.

I created a branch in the example https://github.com/nonameplum/tuist-amplify/tree/fixed that has everything applied.

@stefanomondino
Copy link
Collaborator

I just have a more general question for @danyf90 and the Tuist team.

This "extension" problem with static libraries is not only a problem for Amplify, but can happen to any project and can be REALLY difficult to spot.
Do you think it woud be worth to add that flag (GENERATE_MASTER_OBJECT_FILE) to every Tuist target with static linking? Feels like it could prevent some serious bugs.

The big problem about this is that you won't be able to spot the problem at compile time, but only at runtime... and if you don't have tests around your specific part of code you'll definetly ship a bugged app without noticing or without having it under control.

But I don't know the drawbacks of such flag enabled - what can happen? Why is it off by default on Swift? I can dig into that but if there's anyone with more... compiler experience in the Tuist community it would be great to know

@kwridan
Copy link
Collaborator

kwridan commented Oct 24, 2023

hi @stefanomondino

I can't claim I have any deep insights on this build setting but from a quick test on our project (>200 static targets) I noticed the following:

  • Archiving failed due to some missing symbols (needs some more digging, possibly a misconfiguration somewhere)
  • Debug builds increased in size by ~8%

As such would be hesitant to make it a default :/

@stefanomondino
Copy link
Collaborator

Getting back here - anybody had any luck integrating Amplify after Tuist 4?
With previous (working) configuration I now get lots of Import of shadowed module 'AwsCPlatformConfig'

@stefanomondino
Copy link
Collaborator

Nevermind, seems like Tuist 4.1.1 fixed it somehow. <3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bounty Resolving this issue is rewarded with a bounty: https://github.com/tuist/tuist/discussions/4982 type:need/problem Report needs or problems you came across that are not bugs
Projects
None yet
Development

No branches or pull requests

6 participants