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

Use actual mill version in predef #1307

Merged
merged 1 commit into from Jan 20, 2020
Merged

Conversation

jodersky
Copy link
Contributor

As of recent versions of mill, the $ivy importers support
substituting $MILL_VERSION with the current version of mill.

The changes presented here modify the predef import to use the literal
$MILL_VERSION instead of a the value read from mill's version
file. While both these values are usually equivalent, they can differ
when using a local mill wrapper script (e.g. when developing and
testing mill locally).

In essence, the changes ensure that the correct bloop plugin will
always be used.

@jodersky jodersky requested a review from tgodzik January 14, 2020 23:25
@jodersky
Copy link
Contributor Author

Looks like this is blocked until bloop integration with mill is fixed. I.e. until com-lihaoyi/mill#756 is merged

Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! I was actually hoping for something like this. LGTM

@tgodzik
Copy link
Contributor

tgodzik commented Jan 15, 2020

The tests are failing though

INFO  java.lang.NoClassDefFoundError: io/circe/RootEncoder
INFO  	at mill.contrib.bloop.BloopImpl$BloopOps$bloop$.$anonfun$config$3(BloopImpl.scala:81)
INFO  	at scala.collection.mutable.HashMap.getOrElseUpdate(HashMap.scala:86)
INFO  	at mill.moduledefs.Cacher.cachedTarget(Cacher.scala:12)
INFO  	at mill.moduledefs.Cacher.cachedTarget$(Cacher.scala:10)
INFO  	at mill.define.Module.cachedTarget(Module.scala:18)
INFO  	at mill.contrib.bloop.BloopImpl$BloopOps$bloop$.config(BloopImpl.scala:81)
INFO  	at mill.contrib.bloop.BloopImpl$BloopOps$bloop$.$anonfun$writeConfig$1(BloopImpl.scala:86)
INFO  	at scala.collection.mutable.HashMap.getOrElseUpdate(HashMap.scala:86)

@jodersky any idea what is happening here?

@ckipp01
Copy link
Member

ckipp01 commented Jan 15, 2020

The tests are failing though

I believe this is because com-lihaoyi/mill#756 is merged, but it will need to be released before this will work as expected.

@krotton
Copy link

krotton commented Jan 15, 2020

The fix is currently released in a Mill's snapshot, so changing to:

override def version: String = "0.5.9-6-484657"

fixes the issue immediately.

@jodersky
Copy link
Contributor Author

jodersky commented Jan 15, 2020

I believe this is because com-lihaoyi/mill#756 is merged, but it will need to be released before this will work as expected.

Indeed. I think the best course of action is to wait until a new "official" mill release is out (I reckon it will be 0.5.10). Once it's out I'll update the minimum version in this PR.

Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

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

Let's wait for the stable Mill version.

As of recent versions of mill, the `$ivy` importers support
substituting `$MILL_VERSION` with the current version of mill.

The changes presented here modify the predef import to use the literal
`$MILL_VERSION` instead of a the value read from mill's version
file. While both these values are usually equivalent, they can differ
when using a local mill wrapper script (e.g. when developing and
testing mill locally).

In essence, the changes ensure that the correct bloop plugin will
always be used.
@jodersky
Copy link
Contributor Author

A new stable release of mill is out, I updated the version settings and pushed again. Hopefully this time the build will pass.

Copy link
Member

@ckipp01 ckipp01 left a comment

Choose a reason for hiding this comment

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

Awesome, this looks good. LGTM!

@ckipp01 ckipp01 merged commit d141bad into scalameta:master Jan 20, 2020
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.

None yet

4 participants