Validates that an apple_framework_packaging's minimum_os_version is n…

[congt]

…ot lower than those of its deps

Swiftc has already enforced it at compile time, while objective-c doesn't. Adding this validation to ensure it apply to objective-c frameworks, too.

[justinseanmartin]

Is there a reasonable way to unit test this functionality?

Also, would it be worth perf testing this, or is it not substantial compared to the amount of time compiling? You might be able to leverage the work Dimitris did on Uber Poet to have a substantially sized demo repo.

[justinseanmartin]

I think it should be able to operate only on its direct dependencies, shouldn't it? If something transitive is lower, I'd think it would get flagged by the thing that depends on it directly. I'm just thinking that this might save some traversal time, but I don't know if it is substantial enough to have a notable impact.

[dierksen]

Agreed; since this will check at each level, there's no need to traverse further than one level down at each step.

[congt]

Actually an apple_framework_packaging target doesn't directly depends on other apple_framework_packaging targets if one uses the macro apple_framework see https://github.com/bazel-ios/rules_ios/blob/master/rules/framework.bzl#L74-L117:

def apple_framework(name, apple_library = apple_library, **kwargs):
    ...
    framework_deps = []
    ...
    framework_deps.append(force_load_name)

    framework_deps += library.lib_names
    
    apple_framework_packaging(
        name = name,
        ...
        deps = framework_deps,
        ...
    )

[dierksen]

oh, gross, but since the transitive deps are already being used in a bunch of places, I wouldn't think this would add additional impact then.

[congt]

Yeah. If you look at the implementation, transitive_deps is NOT all transitive deps of the apple_framework_packaging target. It actually only includes all intermediate libraries and the corresponding apple_framework's deps.

[gyfelton]

should we include other reviewers to make sure they are aware of the change? This one can potentially break other ppl's build

[dierksen]

oh, gross, but since the transitive deps are already being used in a bunch of places, I wouldn't think this would add additional impact then.

[congt]

Is there a reasonable way to unit test this functionality?

Also, would it be worth perf testing this, or is it not substantial compared to the amount of time compiling? You might be able to leverage the work Dimitris did on Uber Poet to have a substantially sized demo repo.

We've already had positive test cases, but I'm not sure how to add negative test cases. Others also ask about this in this issue: bazelbuild/bazel#2751

Performance should not be an issue at all. Everything is in memory. We also iterate deps and transitive_deps multiple times in apple_framework_packaging. Also note that apple_framework_packaging's transitive_deps is its transitive deps. It only includes the various intermediate libraries and the corresponding apple_framework's deps.

[jerrymarino]

This looks good overall, my only suggestion is to add an opt-out if people request it. Theoretically we can do that as a followup if anyone complains as this seems reasonable to uphold