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

Bug: duplicate targets in project when a rules_ios apple_framework depends on a package from rules_swift_package_manager #3086

Open
jschear opened this issue Sep 4, 2024 · 13 comments
Labels
bug Something isn't working

Comments

@jschear
Copy link

jschear commented Sep 4, 2024

Description

I'm trying to migrate a project to depend on external swift dependencies via rules_swift_package_manager. When a rules_ios apple_framework depends on a target generated by rules_swift_package_manager, it appears that the target merging logic fails to unify the apple_framework's underlying bazel targets.

Reproduction steps

I have a reproducer using the rules_ios example in this branch: https://github.com/jschear/rules_xcodeproj/tree/js/rules_swift_package_manager_rules_ios_example

  • cd examples/rules_ios and bazel run //:xcodeproj-incremental
  • open the generated project and find the target list
  • MixedAnswer is present as both a library and static framework, and MixedAnswer_objc is present as a library.
    image

Expected behavior

  • MixedAnswer is only present as a static framework (which happens on main)
    image

rules_xcodeproj version

44b6f04

Xcode version

15.4

Bazel version

7.1.1

rules_apple version

3.2.1

rules_swift version

1.16.0

Additional information

No response

@jschear jschear added the bug Something isn't working label Sep 4, 2024
@thiagohmcruz
Copy link
Contributor

cc @luispadron wondering if Cash hit anything similar while working on SPM stuff?

@luispadron
Copy link
Contributor

We haven't gotten that far into testing SPM + rules_ios yet.

I'm out this week but can help look more next. Just to confirm, we can repro this with rules_iOS and rules_spm (not using the fork y'all have)?

@thiagohmcruz
Copy link
Contributor

We haven't gotten that far into testing SPM + rules_ios yet.

I'm out this week but can help look more next. Just to confirm, we can repro this with rules_iOS and rules_spm (not using the fork y'all have)?

Correct, we can repro using the setup Jonathan shared above using the examples in the repo and the upstream rules.

@thiagohmcruz
Copy link
Contributor

cc @brentleyjones I'd appreciate any insight here 🙇

@jschear
Copy link
Author

jschear commented Sep 6, 2024

I can repro the same issue when a rules_ios apple_framework depends directly on a swift_library, without involving rules_swift_package_manager at all: main...jschear:rules_xcodeproj:js/rules_ios_swift_library_merging. I can see that there are three mergeable_infos for the target (the underlying MixedAnswer_swift swift_library, the MixedAnswer_objc objc_library, and the new swift_library), and only targets with two mergeable_infos are merged:

Still ramping up on the codebase, but that's what I've figured out so far.

@brentleyjones
Copy link
Contributor

This seems like a rules_ios bug then? The new swift_library should be a dep of MixedAnswer_swift and/or MixedAnswer_objc, but not the packaging rule or whatever.

On the rules_xcodeproj side I think we can solve this by making sure that if a dep is handled upstream, that we don't deal with it again for merging. I believe it used to do this, so something about generating and handling the mergeable_infos is causing the new swift_library to be handled multiple times and not filtered out.

@jschear
Copy link
Author

jschear commented Sep 18, 2024

Thanks for taking a look at this.

This seems like a rules_ios bug then? The new swift_library should be a dep of MixedAnswer_swift and/or MixedAnswer_objc, but not the packaging rule or whatever.

Very possibly a rules_ios bug. As far as I can tell the swift_library is not present in the deps of the packaging rule, so it's surprising to me that it's showing up in that mergeable_infos list -- my understanding of the code thus far is that only direct dependencies should be included.

bazel query --output=build //iOSApp/Source/CoreUtilsMixed/MixedAnswer:all:

apple_framework_packaging(
  name = "MixedAnswer",
  visibility = ["//iOSApp:__subpackages__"],
  generator_name = "MixedAnswer",
  generator_function = "apple_framework",
  generator_location = "iOSApp/Source/CoreUtilsMixed/MixedAnswer/BUILD:6:26",
  testonly = False,
  framework_name = "MixedAnswer",
  deps = ["//iOSApp/Source/CoreUtilsMixed/MixedAnswer:MixedAnswer_swift", "//iOSApp/Source/CoreUtilsMixed/MixedAnswer:MixedAnswer_objc"],
  private_deps = [],
  library_linkopts = [] + [],
  vfs = [],
  transitive_deps = ["@swiftpkg_swift_log//:Logging"],
  environment_plist = ...,
  bundle_id = "rules-xcodeproj.MixedAnswer",
  platforms = {"ios": "15.0"},
  platform_type = ...,
  minimum_os_version = ...,
)

It is in transitive_deps, but I don't believe anything in rules_xcodeproj checks that attribute.

On the rules_xcodeproj side I think we can solve this by making sure that if a dep is handled upstream, that we don't deal with it again for merging. I believe it used to do this, so something about generating and handling the mergeable_infos is causing the new swift_library to be handled multiple times and not filtered out.

By "handled upstream", do you mean included in the project as a target?

I'll dig into why the swift_library target is getting included as a mergeable_info.

@brentleyjones
Copy link
Contributor

By "handled upstream", do you mean included in the project as a target?

I mean that MixedAnswer_swift is already looking at @swiftpkg_swift_log//:Logging, so it should provide some information of that so that MixedAnswer can ignore it.

@jschear
Copy link
Author

jschear commented Sep 19, 2024

Ah, rules_xcodeproj does actually consider transitive_deps of apple_framework_packaging when merging targets: #2750

I don't have full context on why transitive_deps exists in the first place, but I'm going to check if removing it works.

@brentleyjones
Copy link
Contributor

Hmm, seems like we shouldn't with the new merging logic.

@jschear
Copy link
Author

jschear commented Sep 19, 2024

Shouldn't remove it, or shouldn't consider transitive_deps? 😄

@brentleyjones
Copy link
Contributor

Shouldn’t consider it.

@luispadron
Copy link
Contributor

I think this might be related to #3094 which i just opened (forget about this one). I added a reproducible example in that issue though

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants