-
Notifications
You must be signed in to change notification settings - Fork 109
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
Swift support #96
base: main
Are you sure you want to change the base?
Swift support #96
Conversation
92baa13
to
fc313f7
Compare
Hey, @xinzhengzhang! Thanks for all your good work on this. I remain eager to get swift support in--and this is certainly a simpler diff--but I have some questions.
Sorry to be the one saying that this needs a bit more discussion and work--but I really appreciate your continued efforts! Cheers, |
"Very interesting that there are multiple! I'd have guessed that would have triggered a lot of unnecessary recompilation. If you know why, I'd be very curious." That's how I understand it. Because in a module, swift files are mutually visible and referenced. When any file changes, it is similar to the change of the header file, and other files must be recompiled. Since this is the case, it is reasonable to put all the sources of the entire module in one compilation task. In addition, you can see many parameters features with the module cache keyword, swift itself has been doing a lot of efforts to speed up its incremental compilation speed. As a user, our best practice is to dismantle the library as small as possible and make full use of bazel cache |
fc313f7
to
c6044da
Compare
Thank you for your kindly permission but it is not needed now~ I have already filtered the swift source before sending to the infer.
Yes, I just lost it and not added it.
Done
Because English is not my mother tongue, I can still write some short sentences, but I can't do it when it comes to the whole paragraph... So I may need your help here and I will write some outline about how the things work.
|
c6044da
to
b042f0d
Compare
Hello again @xinzhengzhang! Again, sorry for being slow--got buried in tasks over here. Thanks so much for teaching me more about the swift compilation model. That helps me be more effective, now and into the future. I think we'll still need Thanks so much for working through some of the others! If anything from above remains, could I ask you to work through that, too? (Update: after reading through the code and pushing some minor fixes, some definitely remain. Could I ask you to see how clean you can make things?) On instructions for users: I'm more than happy to convert bullet points into paragraphs and generally handle the English writing part. (But you should feel good about your English! You write in English far better than I write in any other language.) But I will need slightly more detailed instructions. Do users need to manually specify the compile_commands.json mode? Is there a way to remove the dependency on a preexisting build, by, say, removing the -fmodule flags like we do for C++? (If so, we should definitely do that, I think.) And again, is there configuration that's important for Indexing While Building? Thanks, |
For simpler merging with the "file based filter" PR moving to end for greater clarity
Defensive instead of arms length protection All take the compile_acion and return a modified args array.
Ok, I will add exclude_swift and list issue link later
No, they don't. Sourcekit-lsp will be automatically selected according to the files in the current workspace. If there are both
Because swift interacts with C and swift using .module and .swiftmodule. As far as I know I don't know of any other way to get sourcekit to work without precompiling the module. However, we can precompile once in the extractor process like
After my test, no matter whether the index-store of sourcekit-lsp and rule_swift are consistent when compiling, they can work normally. But setting the same store should speed up indexing. (I can make a benchmark after I haven't tested it here) In rules_swift we need to enable features SWIFT_FEATURE_INDEX_WHILE_BUILDING and SWIFT_FEATURE_USE_GLOBAL_INDEX_STORE using |
Sweet. Thanks. x2 I see. Should we automatically kick off a bazel build if there are swift compile actions, then, if there's really no way to skip building & modules? (I didn't quite understand what you were saying about the paths, but maybe you were proposing just generating the module files?) Sounds good. When you're done, could I ask you to add that (and the above) to the instructions bullet points so I can flesh them out? |
Gave this a shot locally but seems like it doesn't quit work:
This is with Bazel 6.0.0 I would love to get support for this landed so let me know if there is anything I can do to help push this forward! |
Because there is only one file in the core of this project, there are many conflicts between the two features (#99 and #96) . I am going to deal with swift after the file-based pr is completed. If you need it, you can use https://github.com/xinzhengzhang/bazel-compile-commands-extractor/commits/feature/bis-support-rb3 this branch which was rebased last week and was used in the vscode plugin https://github.com/xinzhengzhang/bis. As for the patch related to swift, I am also independent it it https://github.com/xinzhengzhang/bis/blob/main/patches/swift_support.patch |
Currently feature/swift-support is broken, but it conflicts too much with feature/file-based-filter. I temporarily rebase the two features into one branch https://github.com/xinzhengzhang/bazel-compile-commands-extractor/commits/feature/bis-support-rb4. |
Agree re ordering. Working on it--sorry for the backlog, guys. |
@xinzhengzhang I tried the bis-support-rb4 branch and it works great on my project, except when I add a library that uses |
I think this might be something with how the platform is detected. I added a config setting like: https://github.com/bazelbuild/rules_swift/blob/61df2d5e9f549fa47bfe38fae7829408d16bafed/examples/xplatform/macros/BUILD#L6-L9 and I get an error like the following:
Should I be adding a flag to the refresh_compile_commands target to set the platform to ios? |
It is true that swift macro is not currently supported. I will see how to support it recently. |
I solved the first issue by modifying the json to remove the |
@joprice Delayed for a few days due to the need to upgrade the swift version I would like to ask what is your environment? Your problem is not reproduced. There is nothing wrong with the code prompt and automatic completion, but there are two flaws found so far.
Maybe we need to wait until swiftlang/sourcekit-lsp#892 this is solved. Test case I have added the case of swift macros to my plugin version, its code should be consistent with bis-support-rb4 I looked at the implementation of rules. This define should be completely unnecessary. It should be placed here mainly for explicit enable.
I didn't reproduce it. Maybe there is a problem with the version of sourcekit-lsp you are using? |
I have the same environment. The only difference I see is freestanding vs attached macros, where I'm using attached ones like |
Reimplement for #89