Skip to content
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

Merge sbt/zinc and typesafehub/zinc #80

Closed
2 of 5 tasks
gkossakowski opened this issue Mar 14, 2016 · 11 comments
Closed
2 of 5 tasks

Merge sbt/zinc and typesafehub/zinc #80

gkossakowski opened this issue Mar 14, 2016 · 11 comments

Comments

@gkossakowski
Copy link
Contributor

This ticket is for discussion and planning of merging functionality of sbt/zinc (known before as sbt/incrementalcompiler) and typesafehub/zinc. The end goal of the merge process is to retire typesafehub/zinc into an attic and have sbt/zinc be the place for incremental compilation for Scala.

An initial list of tasks with owners in parenthesis:

@kiritsuku
Copy link
Contributor

Any plans to work on this or is this just a "would be nice to have" thing right now?

@eed3si9n
Copy link
Member

@sschaef Some of the work is under way. Inputs on any of these parts are welcome while it's jello solid.

@gkossakowski
Copy link
Contributor Author

@sschaef, I'm working on merging class-based-dependencies branch into sbt/zinc. This means, the best implementation of incremental compilation will be available from this repo.

I added publishing to Maven as a task to show that some work has been already done. The rest is looking for people willing to work on it.

@gkossakowski
Copy link
Contributor Author

I should add, that turning sbt/zinc into a module that sbt 1.0-M1 can use is the priority.

@stuhood
Copy link

stuhood commented Apr 5, 2016

Great to see some action over here!

About a year ago, github.com/pantsbuild/pants forked zinc in order to apply a few performance, usability, and release-lifecycle improvements. If there is going to be a revival of zinc, we'd love to get unforked and help out there.

Some relevant reviews; some will not cherry-pick cleanly, so it will likely be on us to try and upstream those. Others would likely be easy/useful to patch in.

The primary reason for the perf reviews is that we have a lot of modules (on the order of ~2000 in some cases) so we run with fairly large analysis caches, and need laziness/soft-references there. Also, pants does its own maintenance of the various working directories (as most other caller would be likely to do: IDEs in particular), and so the analysis hashing that was being done in order to hit the analysis cache was a real bottleneck.

@stuhood
Copy link

stuhood commented Apr 6, 2016

Update: I just finished updating the zinc fork for the most recent APIs... unsure whether it's helpful for you all to see, but it seems that zinc (as it existed a year ago) now requires access to a lot of private APIs: https://rbcommons.com/s/twitter/r/3658/

@gkossakowski
Copy link
Contributor Author

Thanks for chiming in. Zinc is revived now in the form of sbt/zinc and it's a good time now to contribute; you have many eyeballs available for reviews. And multiple people have details of sbt/zinc loaded into their short term memory.

I scanned the patches you linked:

[perf] https://rbcommons.com/s/twitter/r/2827/

The direction of this patch is very good and overlaps with sbt/sbt#2525. The sbt/sbt#2525 takes it further because it skips the entire classpath lookup part of Analysis look. This is relevant for builds with a large classpath. I already applied some of the changes described in the ticket so looking up Analysis is a bit different now compared to what you see in the old zinc or sbt 0.13 and closer to what you have in your patch.

[perf] https://rbcommons.com/s/twitter/r/2190/

I have no opinion to offer; I'm not familiar with this part of the old zinc. I don't think this patch would be problematic to reapply, though.

[perf] https://rbcommons.com/s/twitter/r/2178/

It looks like it's also related to sbt/sbt#2525

[correctness] https://rbcommons.com/s/twitter/r/2867/

No opinion here as I don't know this part of the old zinc.

[feature] https://rbcommons.com/s/twitter/r/2656/ & https://rbcommons.com/s/twitter/r/2666/

These two look fine to me.

@gkossakowski
Copy link
Contributor Author

And regarding cherry-picking from either the old zinc or your fork: I'm fine with you bringing over whatever you need to unfork yourself. I hope there won't be too many pantsosims.

@stuhood
Copy link

stuhood commented Apr 6, 2016

Sounds good. The sooner this ticket results in a binary that we can potentially switch to, the sooner it makes sense for us to begin contributing!

re: sbt/sbt#2525: It certainly looks related to the optimization from https://rbcommons.com/s/twitter/r/2178/ ... the most important part there was eliminating classpath (ie filesystem) lookups in cases where we had analysis.

@kiritsuku
Copy link
Contributor

Did you already publish an artifact or can I publish this locally? I would like to test what happens if we throw away the sbt dependency in Scala IDE and instead depend only on zinc.

@eed3si9n
Copy link
Member

I'm planning to get something out in a few days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants