Add support for swift #89

xinzhengzhang · 2022-11-24T10:00:07Z

Add support for SwiftCompile in rule_swift

It extracts swift compile commands into compile_commands.json providing to sourcekit-lsp

VSCode setup

code --install-extension sswg.swift-lang

Examples:

load("@build_bazel_rules_swift//swift:swift.bzl", "swift_library")
load("@hedron_compile_commands//:refresh_compile_commands.bzl", "refresh_compile_commands")

swift_library(
    name = "Sources",
    srcs = [
        "ViewController.swift",
    ],
    deps = [
         #objc_library
        "//lib:hello",
    ]
)

refresh_compile_commands(
    name = "refresh",
    enable_swift = True,
    targets = {
      "//:Sources": "--apple_platform_type=ios --cpu=ios_arm64",
    }
)

# Create compile_commands.json
bazel run //:refresh
# Build .swiftmodule
bazel build //:Sources --apple_platform_type=ios --cpu=ios_arm64
# Open is vscode
code .

cpsauer

@xinzhengzhang, thanks so much for all your work here, for taking the lead, and for simplifying, unifying the implementations, and posting this. And sorry again for my holiday slowness. You can expect me to be faster in the future.

I'll comment on things that jump out to me as being worth discussing in this first round. Thanks for being great!

cpsauer · 2022-12-02T04:04:59Z

refresh_compile_commands.bzl

@@ -100,6 +102,7 @@ _expand_template = rule(
        "labels_to_flags": attr.string_dict(mandatory = True),  # string keys instead of label_keyed because Bazel doesn't support parsing wildcard target patterns (..., *, :all) in BUILD attributes.
        "exclude_external_sources": attr.bool(default = False),
        "exclude_headers": attr.string(values = ["all", "external", ""]), # "" needed only for compatibility with Bazel < 3.6.0
+        "enable_swift": attr.bool(default = False),


What do you think about just always having swift enabled, eliminating the need for configuration and branching?

I'm asking because swift extraction should be really fast, right, since there's no need for header extraction. Normal clangd doesn't crash or otherwise error out if there are swift commands, right?

You're the expert here, but it seems from the outside like this is worth having on all the time :)

This flag designed for some other tools like infer
In bilibili we are using it for static analysis and it didn't support swift yet...

Ah! Got it. And to confirm, the static analysis tools crash, rather than gracefully ignoring the swift files? Bummer.

How about it we at least turn swift on by default?
Perhaps "exclude_swift" defaulting to False?
(We should make sure to eventually document this in the README and file docstring)

It might also be worth submitting bugs to the tools that crash on swift files? They're going to see more and more of those inputs as the years roll on, I think.

I think you are right, other tools should not transfer the problem to this compatibility to increase the cost of using.
I will add a filter in my case and remove this flag

xinzhengzhang@22317ce

Sounds good. But if it removes the need for the file filter, I'd still be happy with keeping an exclude_swift flag (off by default) for you, until those tools become tolerant to swift files?
(They do crash, yes, or otherwise issue errors that interfere with their use?)

Done
xinzhengzhang@047daf9

refresh.template.py

cpsauer · 2022-12-02T04:27:24Z

refresh.template.py

@@ -683,7 +694,36 @@ def _get_apple_active_clang():
    # Unless xcode-select has been invoked (like for a beta) we'd expect, e.g., '/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang' or '/Library/Developer/CommandLineTools/usr/bin/clang'.


-def _apple_platform_patch(compile_args: typing.List[str]):
+@functools.lru_cache(maxsize=None)
+def _get_apple_active_swiftc():


Just checking my understanding: Does sourcekit-lsp have problems if we don't unwrap the main executable? I (I'm not sure if it's ever executed, like with query-driver in clangd--or how tolerant they are of unknown flags.)

I tried and it does seem to work without unpacking the main executable in sourcekit-lsp but it can not tolerant other unknown flags like -Xwrapped-swift

error: unknown argument: \'-Xwrapped-swift=-debug-prefix-pwd-is-dot\'error: unknown argument: \'-Xwrapped-swift=-ephemeral-module-cache\'")) Code: -32001

And it is still necessary for c&cpp because we need to extract header information.

To be on the safe side, I think it's better to unwrap it

Main executable: After some more thought, I think we should just set it to 'swiftc', rather than finding the full path via xcrun--assuming that works.
Why? This'll keep things more open for Swift on other platforms down the line that don't have xcrun (like Linux, Swift for servers). If a tool does try to resolve swiftc, it should work just fine without xcrun, resolving to the same system default via the path, so we're stripping out unnecessary complexity. This works for clang, too, so I've removed the xcrun call for clang on the main branch in a797a78. Sorry for leading you down an unnecessarily complex path to start; thanks for helping make this tool better all around.

Flags: Just to make sure I'm on the same page, these are red-squiggly errors in the editor, right? Makes total sense that we'll need to remove/replace them, then.

Here we should set clang in else branch or just assert the main executable has to be one of ['swiftc' , 'external/local_config_cc/wrapped_clang'] ?

I have rebased the branch.

Here is the implementation

if compile_args[0].endswith('swiftc'): compile_args[0] = 'swiftc' else: compile_args[0] = 'clang'

refresh.template.py

cpsauer · 2022-12-02T04:37:44Z

refresh.template.py

+        compile_args.insert(match + 1, os.environ["BUILD_WORKSPACE_DIRECTORY"] + "=.")
+
+    # Remove other -Xwrapped-swift arguments like `-ephemeral-module-cache` `-global-index-store-import-path`
+    # We could override index-store by config sourcekit-lsp


What's our plan for integrating their indexing during build?
(Eventually we'll need to document how to configure things in the README, but that can wait until the end.)

We can analyze commands in aquery and pre build .swiftmodule by striping -emit-object in swift but that looks like it's going to be the second super slow header extractor...

For some small project fully build the target and cache the results by bazel ecosystem seems the best way but for some super huge project like bilibili it is impossible to wait a fully build before writing code. So I am doing another set of rules and a vscode plugin for integrating indexing with building. I will sync that once there is any progress

For clarity: I wasn't meaning to propose that we force a full swift build up front when it isn't needed, but we should help people get sourcekit-lsp configured correctly, via instructions in the readme.

I'm guessing part of that will be pointing source kit-lsp to pick up the indexing output generated during bazel builds? And configuring bazel builds to generate that output if they don't already?

I'd love to learn more about what you're working on with the plugin!

Here is my implementation.
I have wrapped the hedron_refresh_compile_commands and collect all the module info which depended by the target into outputs.
Not as painful as rebuilding .d file for find headers, we can share module objects with compiling and indexing in this way.

refresh.template.py

cpsauer · 2022-12-02T04:43:20Z

refresh.template.py

+    """De-Bazel(rule_swift) the command into something sourcekit-lsp can parse."""
+
+    # Remove build_bazel_rules_swift/tools/worker/worker
+    compile_args.pop(0)


Could you explain/doc a little bit more about what's happening here and below? I assume these are things built into the wrapper scripts, maybe with a persistent worker? (Sorry for my ignorance--I just haven't worked much with Bazel&swift)

Got it. Is there a case we need to worry about where the user disables persistent workers?

At least in the current rules_apple version we don't need.
The worker has wrapped both.

cpsauer · 2022-12-02T04:45:32Z

refresh.template.py

@@ -703,7 +748,7 @@ def _apple_platform_patch(compile_args: typing.List[str]):
        # We also have to manually figure out the values of SDKROOT and DEVELOPER_DIR, since they're missing from the environment variables Bazel provides.
        # Filed Bazel issue about the missing environment variables: https://github.com/bazelbuild/bazel/issues/12852
        compile_args = [arg.replace('__BAZEL_XCODE_DEVELOPER_DIR__', _get_apple_DEVELOPER_DIR()) for arg in compile_args]
-        apple_platform = _get_apple_platform(compile_args)
+        apple_platform = _get_apple_platform(compile_args, environmentVariables)


Ooh, are the environment variables specified correctly for the swift toolchain? If so, this'd be interesting to report on the issue linked above, since they'd been asking about whether it was cc specific.

Of course correct, the environment is resolved by the apple_common provider in

https://bazel.build/rules/lib/apple_common#apple_host_system_env
https://bazel.build/rules/lib/apple_common#target_apple_env

cpsauer · 2022-12-02T04:51:10Z

refresh.template.py

+    match = next((i for i,v in enumerate(compile_args) if v == "-Xwrapped-swift=-debug-prefix-pwd-is-dot"), None)
+    if match:
+        compile_args[match] = "-debug-prefix-map"
+        compile_args.insert(match + 1, os.environ["BUILD_WORKSPACE_DIRECTORY"] + "=.")


Like with clang, might be able to just strip this out, since the commands are (conceptually) happening in place, not that we're actually running/debugging with them anyway.

Yes, indeed we are not running but I found that there are a few options for specify index store path and I guess it may be can share the cache with that in build phase if the same store-path was specified.

sourcekit-lsp -h -index-store-path <index-store-path> Override index-store-path from the build system -index-db-path <index-db-path> Override index-database-path from the build system

So I do everything possible to keep the two consistent though not being validated

Those flags look handy for configuring sourcekit-lsp! (like in #89 (comment))

But I just meant that it might make sense just omit that particular flag, -debug-prefix-map, since I don't think it'll impact analysis--nor compilation if it's just the cwd.

Done
xinzhengzhang@facd48e

cpsauer · 2022-12-03T01:24:12Z

Separate question: Do you know how source kit lsp gets its clangd?
(Usually we drop workarounds when clangd has a new release--I want to make sure we can keep doing that.)

xinzhengzhang · 2022-12-03T11:49:18Z

Separate question: Do you know how source kit lsp gets its clangd?
(Usually we drop workarounds when clangd has a new release--I want to make sure we can keep doing that.)

clangd was found by searching the bin path
https://github.com/apple/sourcekit-lsp/blob/main/Sources/SKCore/Toolchain.swift#L123

…/89\#discussion_r1037771523 refresh.template.py: omit warn_missing_generated and rename _file_exists to _warn_if_file_doesnt_exist

…/89#discussion_r1037776844 refresh.template.py: using endwith api

…/89#discussion_r1037780681 refresh.template.py: do not suppress error when locate xcrun toolchain

…/89#discussion_r1037781502 refresh.template.py: add some more explain for why need to remove the worker

…/89\#discussion_r1037771523 refresh.template.py: omit warn_missing_generated and rename _file_exists to _warn_if_file_doesnt_exist

…/89#discussion_r1037776844 refresh.template.py: using endwith api

…/89#discussion_r1037780681 refresh.template.py: do not suppress error when locate xcrun toolchain

…/89#discussion_r1037781502 refresh.template.py: add some more explain for why need to remove the worker

https://github.com/hedronvision/bazel-compile-commands-extractor/pull/89\#discussion_r1037771523 refresh.template.py: omit warn_missing_generated and rename _file_exists to _warn_if_file_doesnt_exist hedronvision#89 (comment) refresh.template.py: using endwith api hedronvision#89 (comment) refresh.template.py: do not suppress error when locate xcrun toolchain hedronvision#89 (comment) refresh.template.py: add some more explain for why need to remove the worker

for more information, see https://pre-commit.ci

xinzhengzhang · 2022-12-14T13:29:47Z

Hi Chris~
Is there any other do I need to do about this pr?

cpsauer · 2022-12-17T08:48:08Z

Hey @xinzhengzhang!

Could I ask you to take one more pass through, seeing if there are further ways to simplify the refresh.template.py changes and generally improving things in any way you see?

I'm wondering if it might simplify things to unify the swift code path further with the cc codepath, just conditioning on action.mnemonic when needed rather than routing--or if they're actually different enough that they should be more fully separate. I'm not sure how many args patches are really shared. It'll be harder for C++ users to update things if the codepaths are too unified, but if they should share most logic, then we want to unify them so swift gets more future improvements. Your call, but I think we should definitely go more to one extreme or the other or at least document things so that we're clear. Please let me know what you think.

Let's also rename _apple_swift_patch -> _swift_patch, if that's OK, since swift may well get use on other platforms eventually. More generally, I'm not sure we can rely on __BAZEL_XCODE in the args, and maybe we can avoid enforcing an ordering on the patches? Perhaps we just only swap in clang if it's not swift--and otherwise rely on the popping logic in _swift_patch?

We're also going to at least need some docs to merge. If you want to just outline them, I can flesh them out, since I think I've got comparative advantage on English. (Not meaning any offense; your English is far better than I would be in any other language.) In particular, I think it'd be good to get instructions for pointing sourcekit-lsp into the accumulating index from building.

I can dive in more tomorrow, but it's getting late enough here that I'd better go to sleep for tonight.

Cheers,
Chris

xinzhengzhang · 2022-12-18T06:46:05Z

Sorry, maybe I misunderstood before #1 (comment) , I thought you wanted to make commands_extractor more general and adapt to more languages.
So I chose to separate the implementation of cc and swift directly from the entrance. However, limited by the structure of the current implementation of rules, I did not split it into multiple python files to make it more modular, but directly differentiated it in refresh.template.py.
Indeed, it looks like neither side is relying on it now...

Honestly, there is not much difference between the two implementations except where to find the source files. I will try to write another version and try not to break the original implementation of refresh.template.py.

cpsauer · 2022-12-18T08:15:56Z

Ah, no sorry. Miscommunication somewhere, maybe. (About to go to sleep but want to catch you fast so I don't waste your time.)

cpsauer · 2022-12-18T08:20:03Z

To make sure we're on the same page: Let's keep things in one file. Definitely looking to generalize things!

I was just wondering if maybe the codepaths could benefit from being merged further. (Truly wondering; unsure how much of the cc flag patching applies to swift.)

xinzhengzhang · 2022-12-18T16:19:09Z

bazel-compile-commands-extractor/refresh.template.py

Line 563 in c6cd079

    
           # Note: Bazel, with its incremental building strategy, should only be compiling one source file per action.

But there are indeed multiple sources in one swift compile action.
And I also confirmed again that features in rules_swift can't be split into compiled single action.
Can I extend here to return multiple sources?

xinzhengzhang · 2022-12-22T07:21:13Z

Because it was re-implemented, I re-opened a PR #96 for the convenience of review

xinzhengzhang mentioned this pull request Nov 24, 2022

[Self-Filed] Could we also support Swift? #1

Open

xinzhengzhang force-pushed the feature/swift-commands-rb2 branch 2 times, most recently from 78500d0 to b7ab9ae Compare November 24, 2022 17:08

cpsauer linked an issue Dec 2, 2022 that may be closed by this pull request

[Self-Filed] Could we also support Swift? #1

Open

cpsauer reviewed Dec 2, 2022

View reviewed changes

xinzhengzhang added a commit to xinzhengzhang/bazel-compile-commands-extractor that referenced this pull request Dec 3, 2022

https://github.com/hedronvision/bazel-compile-commands-extractor/pull…

dac489a

…/89#discussion_r1037776844 refresh.template.py: using endwith api

xinzhengzhang added a commit to xinzhengzhang/bazel-compile-commands-extractor that referenced this pull request Dec 4, 2022

https://github.com/hedronvision/bazel-compile-commands-extractor/pull…

d785abb

…/89#discussion_r1037776844 refresh.template.py: using endwith api

xinzhengzhang and others added 4 commits December 5, 2022 22:39

Switch _warn_if_file_doesnt_exist return values for clarity

968686b

Change enable_swift to exclude_swift and default to False

047daf9

Remove extractor for -Xwrappe-swift=-debug-prefix-pwd-is-dot

facd48e

xinzhengzhang force-pushed the feature/swift-commands-rb2 branch from e96bbbe to facd48e Compare December 5, 2022 14:46

[pre-commit.ci] auto fixes from pre-commit.com hooks

aa1067f

for more information, see https://pre-commit.ci

xinzhengzhang closed this Dec 19, 2022

xinzhengzhang reopened this Dec 19, 2022

xinzhengzhang mentioned this pull request Dec 22, 2022

Swift support #96

Open

xinzhengzhang closed this Dec 22, 2022

xinzhengzhang deleted the feature/swift-commands-rb2 branch February 23, 2023 16:20

Add support for swift #89

Add support for swift #89

Conversation

xinzhengzhang commented Nov 24, 2022

cpsauer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpsauer Dec 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xinzhengzhang Dec 5, 2022 • edited Loading

Choose a reason for hiding this comment

cpsauer commented Dec 3, 2022

xinzhengzhang commented Dec 3, 2022

xinzhengzhang commented Dec 14, 2022 • edited Loading

cpsauer commented Dec 17, 2022

xinzhengzhang commented Dec 18, 2022

cpsauer commented Dec 18, 2022 • edited Loading

cpsauer commented Dec 18, 2022

xinzhengzhang commented Dec 18, 2022

xinzhengzhang commented Dec 22, 2022

cpsauer Dec 2, 2022 •

edited

Loading

xinzhengzhang Dec 5, 2022 •

edited

Loading

xinzhengzhang commented Dec 14, 2022 •

edited

Loading

cpsauer commented Dec 18, 2022 •

edited

Loading