search

terhechte / SourceKittenDaemon

Swift Auto Completions for any Text Editor
MIT License
529 stars 37 forks source link

Add support for Swift 3 and latest SourceKitten #50

Closed nanzhong closed 7 years ago

nanzhong commented 8 years ago

I'm totally new to swift and this codebase, but I've had a lot of trouble getting this working and thought I'd try and contribute an update. I've made a number of big changes, so feel free to tell me my understanding and assumptions of this project are not sound and this this PR isn't even salvageable :).

Things that I've changed:

A number of the dependencies are "almost" swift package manger ready, so I've forked those repos and made the changes necessary for this to work, but I feel like if this is an acceptable approach to updating this project, I can submit PRs to those repos for the changes.

Things are successfully building and completion seems to work (able to get completion results, but have not yet verified whether the results are the expected ones).

I have not put a lot of thought into the changes to make this compilable in Swift 3 so there's obviously a lot of room of improvement and refactoring, which I will be doing in the next little bit, but I thought I'd share my progress so far.

Give it a try swift build.

Would love your feedback on this, and how I could help contribute to this project.

Note: I have not updated the travis config so the build is going to fail

nanzhong commented 8 years ago

Just an update.

I tested completion both manually and via company-sourcekit and it seems to work. albeit it is very slow. cc @nathankot is it expected to have to wait a couple of seconds before completions that appearing? I also noticed that at times *Messages* shows error relating to the request taking too long. This was tested using a smallish iOS project.

I added the xcode project generated by swift package generate-xcodeproj. The changes there include adding a -DSWIFT_PACKAGE to the SourceKitten compiler flags so that it correctly imports clang. It's currently segfaulting when building via xcode on SourcKittenFramework, and I have not had time to debug this. Building via swift build on the cli works fine.

Next I'm going to be bring the old tests back and updating them to swift 3. I don't plan on bringing SwiftCode over though.

nathankot commented 8 years ago

@nanzhong big apologies for the long delay - this has been left unread in my mailbox for far too long :) These changes look well thought out 👍

I'm a little bit wary of the giant PR and feel like some of these things could be split up, but there's not need to do that at this point. I would suggest we keep a swift3.0 branch on this repo, mention it in the readme and merge it into master down the road.

I just pushed a swift-3.0 branch, could you change this PR's base to that branch? Then I will be happy to merge ^^

Again, thanks so much for your contribution!! Swift 3 is definitely long overdue for this project

nanzhong commented 8 years ago

No problem, I'm currently working on bringing the tests back in and fixing travis. Maybe hold off the merge until then?

rudedogg commented 8 years ago

I'm sure everyone knows, but just incase SourceKitten released 0.15.0: https://github.com/jpsim/SourceKitten/releases/tag/0.15.0

Should master be Swift 3 once this gets merged?

nathankot commented 8 years ago

No problem, I'm currently working on bringing the tests back in and fixing travis. Maybe hold off the merge until then?

@nanzhong Let me know when you make progress on this :)

No problem, I'm currently working on bringing the tests back in and fixing travis. Maybe hold off the merge until then?

@rudedogg we're still pinned against 0.11.0 so it shouldn't make a difference whether or not SourceKitten releases the swift-3 version - but it'd be nice to move towards this quickly!

nanzhong commented 7 years ago

I have not forgotten about this, been very busy with work this week so I haven't gotten around to this yet, but I plan on getting my changes in this weekend. I might bump the SourceKitten version to the latest as well.

nathankot commented 7 years ago

Thanks @nanzhong no rush!

nanzhong commented 7 years ago

@nathankot I've updated this PR with the the old tests and also a travis config (big thanks for https://github.com/vapor/vapor's work in creating a swift ci script) that works with the swift command line tooling. There is also a swift package manager generated xcode project that can be used for building/testing. I've also bumped all the dependencies to their latest versions.

I noticed a couple of things while wrapping this PR up:

  1. There are some assumptions around the first build target being the target "main" target. eg https://github.com/nanzhong/SourceKittenDaemon/blob/d68534c8a3282e2ca65a0f099521d0bc26c30621/Sources/SourceKittenDaemon/Project/Project.swift#L86. This could just be me not being familiar with this project's implementation and history, but this feels strange.
  2. When running the tests via swift test there are two env vars that need to be set. https://github.com/terhechte/SourceKittenDaemon/pull/50/files#diff-354f30a63fb0907d4ad57269548329e3R9
  3. I've also added linux to the travis build, currently it's failing but with SourceKitten already supporting linux I feel like SourceKittenDaemon should also try and get linux support working.
rudedogg commented 7 years ago

Hey! I wanted to try and help and maybe found a few places the auto-converter made some weird casts (but don't really matter). There are also a few like this in the tests. I could just be missing something though.

Thanks for doing this @nanzhong. It looks like it was a ton of work.

nanzhong commented 7 years ago

@rudedogg thanks for going through this. You're definitely right, these seem to all be things introduced from the auto-conversion to swift 3. Going to go through and try and clean some of this up later tonight.

nanzhong commented 7 years ago

@rudedogg I've addressed your comments and also cleaned up a few others I found. There are probably more :P, but let's tackle those after this gets merged?

@nathankot could you go through this again and let me know if there is anything else that needs to be done to get this in? Hopefully once this lands, there won't be any more giant PRs hehe...

nathankot commented 7 years ago

@nanzhong awesome work! sorry it took so long for me to get around to looking at this @rudedogg thanks for taking a look and going through this!

So I've cloned the repo and noticed a few things:

Builds perfectly using swift build, I've also tried it out in a project with company-sourcekit and everything works fine 👍

Can't seem to build using the .xcodeproj, it gives me these types of errors:

unknown>:0: error: no such file or directory: '/Users/nathan/Development/src/SourceKittenDaemon/Packages/Console-1.0.0/Sources/Console/Bar/Bar.swift'

I believe the reason is because the console package has been updated to 1.0.1 which my machine pulled down, however within the Xcode project it is still hardcoded to looking for the package in the 1.0.0 folder.

Alternatively we can simply do away with the Xcode files and all of its artifacts now that the swift package manager is all we need - what do you think?

Thanks so much for this @nanzhong and sorry for my super slow responses, I'm happy to merge this as soon as the import path issue is solved (which I believe should also solve the Makefile not working for me.)

nanzhong commented 7 years ago

@nathankot thanks for the review. I agree about removing the Xcode project since it's actually auto-generated via swift package manager. The version mismatch is because I generated that project when I have 1.0.0 of that package locally.

nanzhong commented 7 years ago

Oh whoops... I forgot that the tests actually rely on the project being present.... mhmmm... I wonder if it's possible to just have the project be generated at build time so that it doesn't need to be in the repo, but will be available when ci runs.

nanzhong commented 7 years ago

Ok, auto-generating the project at build time won't work because of the aforementioned assumption about the ordering of the targets. So instead I'm going to create a test Xcode project that will be used specifically for testing. This should work for the time being until we can look into how to remove the assumption regarding the ordering of the targets.

nanzhong commented 7 years ago

@nathankot would you mind taking another look? I think it should be okay now. :)

nathankot commented 7 years ago

instead I'm going to create a test Xcode project that will be used specifically for testing.

👍 had another look and I made a PR to your branch that fixes up the Makefile @ nanzhong/SourceKittenDaemon#1

🍾 amazing work @nanzhong!! thanks so much

nanzhong commented 7 years ago

Sweet, merged on my branch.

nathankot commented 7 years ago

Awesome! I think this is in a good state to merge directly into master, could you update the base branch of this PR? =D

nanzhong commented 7 years ago

Done.

nathankot commented 7 years ago

Woot! 🎉