Pass imported swiftmodules to ld with -add_ast_path flag

[kastiglione]

When using imported swift frameworks, and when building dbg, pass the .swiftmodule files to the linker using the -add_ast_path. This enables lldb expression evaluation using such modules.

[kastiglione]

This currently depends on #566. View 222c7fb for the specific changes.

[kastiglione]

I assume support needs to be added for apple_dynamic_framework_import too. I will add dynamic support and tests if this looks right.

[kastiglione]

I've rebased and modified on top of #566.

[kastiglione]

• Decide how to handle dbg/fastbuild logic duplicated here and in rules_swift
• Same, but for -add_ast_path
• Is handling only the "legacy" naming convention ok?
• Update tests to confirm the -add_ast_path was applied to the binary

[kastiglione]

I changed approach slightly. Instead of implementing this logic all inline, I duplicated the interfaces from rules_swift that we'd like to ultimately reuse, and customized them for this case. This makes the callsite cleaner, and makes future migration – should those APIs become public, easier. It also calls out explicitly that this behavior is duplicated, as a reminder that hopefully one day we can reuse this behavior from rules_swift.

[sergiocampama]

I'm ok with supporting only the legacy behavior for now. If adding a test is not too complex, could you add one?

[kastiglione]

Yep, was intending to add tests. Will do that now.

[kastiglione]

@sergiocampama I'm considering making a patch for this locally, but I wanted to check if you had any additional feedback.

[sergiocampama]

also, could you rebase as well? there are some changes that fix the buildifier check

[kastiglione]

Thanks for the review.

[kastiglione]

Fixed

[kastiglione]

Do you want me to rebase again?

[kastiglione]

Rebased.

[allevato]

Also re-ran test/testdata/frameworks/update_frameworks.sh to generate a 5.1 swiftmodule in the test fixtures.

Having a prebuilt .swiftmodule file in there isn't going to work; it version-locks the test to a specific version of Xcode, and makes it impossible for Buildkite to update Xcode without breaking the test. It also means we can't have Buildkite and Google's internal CI running different versions of Xcode, which is a non-starter.

(And this is why I'm nervous about this whole feature to begin with.)

I don't have a great answer for this. Since you actually need it to be a valid module (because you're building code that depends on it), you can't just have it be a dummy file. @thomasvl, do you have any ideas?

[kastiglione]

For what it's worth, master already is in this state.

I could look to generate these fixtures pre- test.

[allevato]

Ah, so it is... from the diff on the files changed page I couldn't tell if it was being added or just 100% changed.

I wonder if you could do this as two phases in the test. First, have a swift_library fixture that you build and that creates a module, then you copy that module into the test's workspace and write out a scratch framework import target in a BUILD file in the same test area, pointing to that module? If that works, that would guarantee they're always the same compiler.

[kastiglione]

Sounds good

[kastiglione]

iOSSwiftStaticFramework.framework is now dynamically generated from the test. This removes the precompiled .swiftmodule. See 469a65e

Also rebased.

[allevato]

I think we're good to go now!

[kastiglione]

Thanks for helping us get this shaped up :)