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

Propogate framework_includes #697

Merged

Conversation

qyang-nj
Copy link
Contributor

Propagate CcInfo.compilation_context.framework_includes in apple_framework_packaging. This is to address a problem described here.

@mattrobmattrob
Copy link
Collaborator

mattrobmattrob commented Apr 19, 2023

Likely not ran into by most users since this style of approach is used for prebuilt frameworks/XCFrameworks:

apple_framework(
    name = "Instabug",
    platforms = {"ios": "10.0"},
    sdk_dylibs = ["z"],
    sdk_frameworks = [
        "AVFoundation",
        "CoreData",
        "CoreGraphics",
        "CoreMedia",
        "CoreMotion",
        "CoreTelephony",
        "CoreVideo",
        "SystemConfiguration",
        "UIKit",
    ],
    vendored_xcframeworks = [
        {
            "name": "Instabug",
            "slices": [
                {
                    "identifier": "ios-arm64_i386_x86_64-simulator",
                    "platform": "ios",
                    "platform_variant": "simulator",
                    "supported_archs": [
                        "arm64",
                        "i386",
                        "x86_64",
                    ],
                    "path": "Instabug.xcframework/ios-arm64_i386_x86_64-simulator/Instabug.framework",
                    "build_type": {
                        "linkage": "dynamic",
                        "packaging": "framework",
                    },
                },
                {
                    "identifier": "ios-arm64_armv7",
                    "platform": "ios",
                    "platform_variant": "",
                    "supported_archs": [
                        "arm64",
                        "armv7",
                    ],
                    "path": "Instabug.xcframework/ios-arm64_armv7/Instabug.framework",
                    "build_type": {
                        "linkage": "dynamic",
                        "packaging": "framework",
                    },
                },
            ],
        },
    ],
    visibility = ["//visibility:public"],
    xcconfig = {
        "FRAMEWORK_SEARCH_PATHS": ["/Instabug/"],
        "APPLICATION_EXTENSION_API_ONLY": "YES",
    },
)

Or like this:

apple_framework(
name = "Basic",
# Note: it is totally possible that a user would write a glob like this..
# srcs = glob([
# "sources/Basic/**/*.h",
# ]),
objc_copts = ["-fmodules-disable-diagnostic-validation"],
platforms = {"ios": "12.0"},
vendored_static_frameworks = ["ios/Basic.framework"],
visibility = ["//visibility:public"],
)

@mattrobmattrob
Copy link
Collaborator

Oh! Have you tried @rules_ios//rules:apple_patched.bzl's apple_static_framework_import for this fix, @qyang-nj? For example:

load(
"@build_bazel_rules_ios//rules:apple_patched.bzl",
"apple_static_framework_import",
)
apple_static_framework_import(
name = "GoogleMobileAds",
framework_imports = glob(
["Frameworks/GoogleMobileAdsFramework-Current/GoogleMobileAds.xcframework/ios-arm64_x86_64-simulator/GoogleMobileAds.framework/**"],
allow_empty = False,
),

@qyang-nj
Copy link
Contributor Author

qyang-nj commented Apr 19, 2023

Interesting! I'm not aware of vendored_xcframeworks or apple_patched.bzl.

Is there any reason we create a new apple_static_framework_import in rules_ios rather than supporting the one from rules_apple?

Also, what's difference between using deps vs vendored_xcframeworks?

@qyang-nj
Copy link
Contributor Author

Oh, I just saw this PR. It looks having apple_**_framework_import_patched is to solve the same problem I'm running into here. But I think my approach is simpler and support apple_**_framework_import from rules_apple. 😅

@mattrobmattrob
Copy link
Collaborator

@jerrymarino, do you have any context on whether we could get rid of @rules_ios//rules:apple_patched.bzl's apple_static_framework_import in favor of this change?

That stuff is all before my historical time. To be fair, I'm okay adding this assuming it doesn't break stuff in other projects though, doesn't seem unreasonable since includes is forwarded already.

@luispadron
Copy link
Collaborator

I'd add a simple fixture exercising the issue to catch regressions but otherwise lgtm

@qyang-nj
Copy link
Contributor Author

@mattrobmattrob @luispadron Any chance we can merge this PR? (I don't have the permission.)

I also tried adding some tests, but it seems currently we have some issues with prebuilt frameworks. I feel it's better to address those issues in a separate PR. Thanks!

@mattrobmattrob mattrobmattrob merged commit 3884a20 into bazel-ios:master Apr 24, 2023
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.

3 participants