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

Beginning of Bloop integration #595

Merged
merged 17 commits into from
Apr 25, 2019
Merged

Beginning of Bloop integration #595

merged 17 commits into from
Apr 25, 2019

Conversation

Baccata
Copy link
Contributor

@Baccata Baccata commented Apr 15, 2019

This is the first of a series of PRs that aims at giving mill modules the ability to delegate some tasks to bloop. I have some more complex stuff lined up, but thought it'd be easier to split the work into smaller PRs.

I initially had started the integration against the bloop repository, but mill's API evolving faster makes it hard to ensure that the integration remains up to date, and I believe this as its place here (happy to have the debate)

This first PR does the following:

  • ports the work currently present in the bloop repo, allowing for the generation of the bloop config from mill, in a way fairly similar to how intellij config is currently generated.
  • adds a BloopModule trait that can be mixed in a mill's JavaModule, giving the ability to call mill module.bloop.config andmill module.bloop.compile. (edit : compiling with bloop will occur in a subsequent PR)
  • updates the versions of the libraries involved to their latest (ujson-circe)

The aim of this first PR is to lay the basis for the integration. It does not address scalajs/scalanative specific usecases. In subsequent PRs, I'm planning to use a mill worker to maintain a bsp connection between mill and bloop to allow the implementation of more complex tasks (testing in particular, the results of which cannot be easily retrieved via mere shellout).

The BloopModule trait does not extend ScalaModule (nor JavaModule). This is to build on the existing modules and let the users decide whether the want to override their compile tasks by calling bloop.compile, or whether they want to retain the freedom to call either tasks.

I haven't added unit-tests yet, and would appreciate some guidance around testing : should I just follow what's been done around the GenIdea module ? Unit tests added

@Baccata
Copy link
Contributor Author

Baccata commented Apr 15, 2019

Pinging @jvican for obvious reasons
@robby-phd this might be of interest to you as well

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.

Do you mind to add some (minimal) documentation about the plugin to docs/pages/9 - Contrib Modules.md? At the current state the most interesting info a user seeks might be the status (what works, what not) and the relation to the mill plugin in the bloop repo (which one is newer).

@Baccata
Copy link
Contributor Author

Baccata commented Apr 16, 2019

@lefou sure, will do 👍. Could you also provide guidelines regarding testing ? What tests would you expect to see in this PR ? Should I follow the GenIdea tests ?

@lefou
Copy link
Member

lefou commented Apr 16, 2019

For testing, we need to cover the file generator (which is like the GenIdeaTests test) and the compilation with bloop (like HelloWorldTests or HelloJavaTests). We should avoid testing bloop itself, but should ensure we cover communication and error handling.

@lefou
Copy link
Member

lefou commented Apr 16, 2019

If you need further help with initial test setup, have a look at other contrib project or ask on gitter.

@Baccata
Copy link
Contributor Author

Baccata commented Apr 16, 2019

and the compilation with bloop (like HelloWorldTests or HelloJavaTests

Gotcha. In the interest of keeping PRs small(ish), I'll remove the compilation task and will add it to a subsequent PR with the relevant tests. Thanks for the pointers.

@@ -12,12 +12,12 @@ object TestEvaluator{
val externalOutPath = os.pwd / 'target / 'external


def static(module: TestUtil.BaseModule)(implicit fullName: sourcecode.FullName) = {
def static(module: => TestUtil.BaseModule)(implicit fullName: sourcecode.FullName) = {
Copy link
Contributor Author

@Baccata Baccata Apr 21, 2019

Choose a reason for hiding this comment

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

This is needed to accommodate a cyclic reference (BloopImpl requires an evaluator contains a path-dependant trait ) (see https://github.com/lihaoyi/mill/pull/595/files#diff-e6a90b093d6e78436bdcedf02eab38dcR23)

@Baccata
Copy link
Contributor Author

Baccata commented Apr 21, 2019

@lefou, I'm happy for this PR to be reviewed

* Resolves artifacts using coursier and creates the corresponding
* bloop config.
*/
def artifacts(repos: Seq[coursier.Repository],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Calling coursier directly as the logic needs to retain the dep => path relation

Copy link

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

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

Very cool!

//////////////////////////////////////////////////////////////////////////////

// Version of the semanticDB plugin.
def semanticDBVersion: String = "4.1.0"

Choose a reason for hiding this comment

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

Metals currently uses 4.1.4

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bumped: a6bbfe8

```scala
// build.sc (or any other .sc file it depends on, including predef)
// Don't forget to replace VERSION
import $ivy.`com.lihaoyi::mill-contrib-bloop:VERSION`

Choose a reason for hiding this comment

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

Is it possible to programmatically run this setup? For example mill --extra-predef /tmp/metals.sc mill.contrib.Bloop/install. It would be nice if we could support mill in the "import build" command in Metals, the same way it works for sbt.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just had a go at it : mill --predef /tmp/metals.sc mill.contrib.Bloop/install would work

Choose a reason for hiding this comment

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

Cool, we're working on adding automatic "import build" for Gradle in Metals, which requires abstracting some sbt-specifics away. I'm optimistic it won't be too much effort to add automatic "import build" for Mill after that.

@robby-phd
Copy link
Collaborator

@Baccata Similar to the summary on Metals support, it would be great if the documentation also summarizes IntelliJ integration (as an alternative to mill's GenIdea) with regards to importing mill project into IntelliJ, compilation on change via Bloop, running/debugging apps/tests, etc., and perhaps refer to the documentation on IntelliJ's support for BSP/Bloop for more information.

@Baccata
Copy link
Contributor Author

Baccata commented Apr 23, 2019

Similar to the summary on Metals support, it would be great if the documentation also summarizes IntelliJ integration (as an alternative to mill's GenIdea) with regards to importing mill project into IntelliJ, compilation on change via Bloop, running/debugging apps/tests, etc., and perhaps refer to the documentation on IntelliJ's support for BSP/Bloop for more information.

In all frankness, I'm not planning on testing any of that myself... at least not before the Intellij-Bloop integration gets out of EAP and reaches mainstream releases. I'd rather someone who's actively interested in those things try them out and see if they work, and amend the documentation after doing so.

@robby-phd
Copy link
Collaborator

Hmm... I'm currently using IntelliJ 2019.1.1 with IntelliJ Scala 2019.1.7, and I can see that it has BSP and Bloop in the build tool configuration. I think those are mainstream releases; anyway, just a suggestion.

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.

Nice PR! Good work. I've not actually executed/tested this PR. Beside that, LGMT.

contrib/bloop/src/mill.contrib.bloop/BloopImpl.scala Outdated Show resolved Hide resolved
contrib/bloop/src/mill.contrib.bloop/BloopImpl.scala Outdated Show resolved Hide resolved
@lefou
Copy link
Member

lefou commented Apr 25, 2019

Great, I'm going to merge. @Baccata What do you think about your commit history? Does keeping all commits make sense, or should I squash?

@Baccata
Copy link
Contributor Author

Baccata commented Apr 25, 2019

I won't be offended if you squash them, most of the commits are clean but I did mess a couple of them

@lefou lefou merged commit b2d480b into com-lihaoyi:master Apr 25, 2019
@lefou lefou added this to the after 0.3.6 milestone Apr 25, 2019
@Baccata Baccata deleted the bloop-pr branch April 25, 2019 12:38
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.

4 participants