Multiple swift_library transitions results in duplicate linkopts

[keith]

With this dependency graph:

Where opt_wrapper applies a transition to lib2 (a swift_library) the result is there are 2 different @build bazel rules swift local config//:toolchain targets with different transitions. Because of this the final linking action has duplicate info from these, such as -rpath /usr/lib/swift -rpath /usr/lib/swift. In the past this hasn't been an issue, but with the new linker in Xcode 15 this produces a warning. Considering the options are the same, even though the configurations are different I'm surprised these aren't being de-duplicated somehow.

This can be reproduced with this project on macOS: repro.zip using bazel build main --linkopt=-v and inspecting that the linker invocation has duplicate -rpath /usr/lib/swift arguments. Or using Xcode 15 beta 5 you can pass --linkopt=-Wl,-fatal_warnings and it will fail because of these duplicate arguments.

[keith]

Not specific to swift, filed bazelbuild/bazel#19103

[fmeum]

Would it be possible to move the dependency on the toolchain library to swift_binary (assuming that's a thing)? That's how cc_binary handles the build-wide malloc and extra link library: https://github.com/bazelbuild/bazel/blob/6f3789aa964875b94aaa5876e4d657ce819353fa/src/main/starlark/builtins_bzl/common/cc/cc_binary_attrs.bzl#L73

[keith]

It wouldn't be since the top level binary might not be a swift_binary, it can also be an objc_library (with some magic linking), cc_binary, or anything else

[fmeum]

I feared that that might be the case. This is indeed tricky if you don't control the top-level rule. CC @gregestren, this is an interesting case where a "reset this dep to a certain top-level config" primitive would help.

[gregestren]

Considering the options are the same, even though the configurations are different

Can you clarify this comment? You mean -rpath /usr/lib/swift is the same, repeated twice? Not that the build options corresponding to the configuration are the same?

Which flags did the transition change?

[keith]

You mean -rpath /usr/lib/swift is the same, repeated twice?

Yes, the command line literally has: -objc_abi_version 2 -rpath /usr/lib/swift ... -objc_abi_version 2 -rpath /usr/lib/swift (which theoretically could be fine, but now Apple is updating the linker to be more strict about it)

Not that the build options corresponding to the configuration are the same?

Right, the configuration has to be different somehow, even if the difference isn't important to the toolchain target, in my repro case here it changes the compilation mode to opt:

def _force_opt_impl(settings, _attr):
    return {
        "//command_line_option:compilation_mode": "opt",
    }

[keith]

Any other bright ideas for this one?

[gregestren]

I'm not sure how the Swift logic sets and gets the linkopts info (the linkopts are set in a swift_toolchain?).

But going by bazelbuild/bazel#19103 (comment) - and making this about pure C++ - I'd hope that whatever C++ logic propagates up linkopts could deduplicate. I can't imagine that's very hard technically. I'm not sure it's safe though, as I think parameter ordering can matter in weird ways.

Toolchains intentionally take the target configuration because they could potentially generate libraries that link into the target binary: https://bazel.build/extending/toolchains#toolchains_and_configurations. So the fact you get two toolchain instances is by design.

I'm not sure what a "revert to top-level config" scenario looks like in this case. Or if the C++ toolchain needs the target configuration as described in the last link.

It's hard for me to think of an answer that isn't specific to C++ logic.

[gregestren]

It's hard for me to think of an answer that isn't specific to C++ logic.

i.e. "C++ linkopt logic intelligently deduplicates".

[chiragramani]

@gregestren @fmeum
Regarding the concern about safety due to potential issues with parameter ordering, I agree that it's a valid concern. However, could we consider implementing a solution on an opt-in basis?This could potentially encompass:

1. Allowing to opt in for duplication.
2. or maybe allowing to opt in for duplication of only certain flags(where we know that for those flags or given our setup, this is completely safe to do so), which can be detailed in .bazelrc.
3. or maybe this being objc specific(for linking context - a bit outside the scope of this thread) as the dedup happens for frameworks and weak_sdk_framework already within Bazel here, [it may not be an ideal solution though, as I would expect merging of cc_info linking context logic to deduplicate this better to benefit other edges, but just sharing few findings]:

• If using Bazel 7, https://github.com/bazelbuild/bazel/blob/release-7.1.0rc2/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java
• If on Bazel 8, or whenever the new starlark objc world is ready, https://github.com/bazelbuild/bazel/blob/a4f78d08792c5e65bc5261c2f7020298cb554c8e/src/main/starlark/builtins_bzl/common/objc/compilation_support.bzl#L711

[fmeum]

The duplication is happening because the C++ rules deduplicate the list of linkopts by reference, not by value, which does make sense since linker flags can cancel each other based on order.

This is a tricky situation since it's entirely possible that a transition could affect the linkopts of the single unconfigured target. What should happen in that case? This sounds like the build graph equivalent of an ODR violation to me, so deduplication may end up masking situations that are straight up bugs.