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

Bump zinc to 1.4.4 #1094

Merged
merged 6 commits into from
Jan 9, 2021
Merged

Bump zinc to 1.4.4 #1094

merged 6 commits into from
Jan 9, 2021

Conversation

adpi2
Copy link
Contributor

@adpi2 adpi2 commented Jan 4, 2021

@lefou
Copy link
Member

lefou commented Jan 4, 2021

Unfortunately, it's isn't as easy as just bumping it. zinc API changed after 1.4.0-M1 and introduced some virtual file abstraction. The required change to mills code base isn't trivial, at least not if your're not familiar with zinc API.

@adpi2 adpi2 force-pushed the bump-zinc branch 4 times, most recently from ab68e33 to 62affda Compare January 7, 2021 15:09
adpi2 added 5 commits January 7, 2021 16:15
Each version of zinc comes with its own compiler bridges on every
version of Scala. Given that two different versions of Mill can use
two different versions of Zinc we must classify zinc workers by zinc
version (in addition to scala version).
@adpi2
Copy link
Contributor Author

adpi2 commented Jan 7, 2021

This PR is ready for review.

It bumps the version of Zinc but it does not take advantage of the new Zinc features: VirtualFiles and Stamps.

A few remarks:

  • The zinc compiler-interface now depends on the Scala library. It cannot be loaded globally in the ZincWorkerModule anymore. So it is loaded once for each scala version.
  • Zinc cannot reuse the compiler bridges compiled by older version of Mill. So I classified them by the version of Zinc. The compiler bridge is now stored in out/mill/scalalib/ZincWorkerModule/worker/dest/zinc-<zincVersion>/<scalaVersion>.
  • The current version of Zinc is incompatible with 2.12.0 but not with 2.12.1.
  • The compiler bridge now depends on its resources to load itself

@adpi2 adpi2 marked this pull request as ready for review January 7, 2021 15:46
@lefou
Copy link
Member

lefou commented Jan 9, 2021

@adpi2 Thank you very much for this upgrade. The change looks good to me after a first glace, but I'd like to give it a second view. I have some questions, see below.

This PR is ready for review.

It bumps the version of Zinc but it does not take advantage of the new Zinc features: VirtualFiles and Stamps.

I have no idea, what we miss because of this. Could you point out what benefits we could gain? Do you see any low hanging fruits?

A few remarks:

  • The zinc compiler-interface now depends on the Scala library. It cannot be loaded globally in the ZincWorkerModule anymore. So it is loaded once for each scala version.

If I understand correctly, we still load and share the zinc worker globally for modules with the same Scala version, right?

  • Zinc cannot reuse the compiler bridges compiled by older version of Mill. So I classified them by the version of Zinc. The compiler bridge is now stored in out/mill/scalalib/ZincWorkerModule/worker/dest/zinc-<zincVersion>/<scalaVersion>.

  • The current version of Zinc is incompatible with 2.12.0 but not with 2.12.1.

What's the impact of it? If it only means we cannot use 2.12.0 this might be ok, as long as we print a proper error message indicating the issue and the solution in case a module wants to use 2.12.0. Are we still able to compile older Scala versions, e.g. 2.11 ?

  • The compiler bridge now depends on its resources to load itself

Can you elaborate what this means?

@adpi2
Copy link
Contributor Author

adpi2 commented Jan 9, 2021

I have no idea, what we miss because of this. Could you point out what benefits we could gain? Do you see any low hanging fruits?

The purpose of VirtualFile and Stamps is to enable remote caching. It makes possible to download a the compilation result, including the zinc analysis file, produced on one computer to benefit from the incremental compilation on another computer.

In terms of implementation, it requires configuring the FileConverter and the StampReader properly. But then it is only useful if there is an infrastructure to publish and retrieve the cache.

If I understand correctly, we still load and share the zinc worker globally for modules with the same Scala version, right?

Yes. The zinc worker is a singleton and it creates only one compiler bridge for modules with the same Sala version.

What's the impact of it? If it only means we cannot use 2.12.0 this might be ok, as long as we print a proper error message indicating the issue and the solution in case a module wants to use 2.12.0. Are we still able to compile older Scala versions, e.g. 2.11 ?

It's only incompatible with 2.12.0. Scala 2.10 and 2.11 can still be used. I pushed a commit to improve the error message

@adpi2
Copy link
Contributor Author

adpi2 commented Jan 9, 2021

Can you elaborate what this means?

Mill downloads the source of the compiler-bridge, then compiles it and loads the produced class files. But since Zinc 1.4.0 the compiler-bridge jar also contains some resource files that must be copied to the destination folder so that the Zinc class loader can find them. Here are the resource files: https://github.com/sbt/zinc/tree/develop/internal/compiler-bridge/src/main/resources/META-INF/services

@lefou
Copy link
Member

lefou commented Jan 9, 2021

I have no idea, what we miss because of this. Could you point out what benefits we could gain? Do you see any low hanging fruits?

The purpose of VirtualFile and Stamps is to enable remote caching. It makes possible to download a the compilation result, including the zinc analysis file, produced on one computer to benefit from the incremental compilation on another computer.

In terms of implementation, it requires configuring the FileConverter and the StampReader properly. But then it is only useful if there is an infrastructure to publish and retrieve the cache.

Ok, so we can safely live without that for now. It will be relevant for #726 though.

Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me.

@lefou lefou merged commit 79937c3 into com-lihaoyi:master Jan 9, 2021
@lefou lefou added this to the after 0.9.4 milestone Jan 9, 2021
@lefou
Copy link
Member

lefou commented Jan 9, 2021

Thank you, Adrien!

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

Successfully merging this pull request may close these issues.

2 participants