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

Add objc_library binary to outputs of apple_framework_packaging rule when virtualized frameworks feature is enabled #931

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jschear
Copy link
Collaborator

@jschear jschear commented Dec 16, 2024

This fixes an issue where building an apple_framework_packaging rule with the VFS feature enabled wouldn't trigger objc compilation for the framework.

When the VFS feature is disabled, the binaries from the objc_library and swift_library targets are merged using libtool, and that binary is included in the outputs of the apple_framework_packaging rule (see packaging action).

When the VFS feature is enabled, all of the packaging actions are skipped, and the binaries from the objc_library and swift_library targets aren't merged. One of the input binaries is arbitrarily included in the rule's output files -- for a target containing swift and objc sources, this would be the swift_library binary.

The PR adds both input binaries to the rule outputs when the VFS feature is enabled, so building an apple_framework_packaging rule triggers both objc and swift compilation actions.

Copy link
Collaborator

@luispadron luispadron left a comment

Choose a reason for hiding this comment

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

Makes sense thank you!

Copy link
Contributor

@thiagohmcruz thiagohmcruz left a comment

Choose a reason for hiding this comment

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

LGTM! Thx for digging on this one!


virtualize_frameworks = feature_names.virtualize_frameworks in ctx.features
if virtualize_frameworks:
return inputs
Copy link
Contributor

@gyfelton gyfelton Jan 9, 2025

Choose a reason for hiding this comment

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

before this change, swiftmodule_out equals to [swiftmodule_in] when VFS is enabled
with this PR, swiftmodule_out equals to swiftmodule_in instead when VFS is enabled. Am I understanding this diff correctly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think swiftmodule_out is equal to swiftmodule_in both before and after this change.

Before:

swiftmodule_out = _framework_packaging_single(ctx, "swiftmodule", [swiftmodule_in], swiftmodule_out, framework_manifest)

def _framework_packaging_single(ctx, action, inputs, output, manifest = None):
    outputs = _framework_packaging_multi(ctx, action, inputs, [output], manifest = manifest)
    return outputs[0] if outputs else None

def _framework_packaging_multi(ctx, action, inputs, outputs, manifest = None):
   # ...
    virtualize_frameworks = feature_names.virtualize_frameworks in ctx.features
    if virtualize_frameworks:
        return inputs
   # ...

_framework_packaging_multi just returns the inputs list, and _framework_packaging_single returns the first item of that list. Does that make sense?

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.

4 participants