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

Resolve Mill build source dependencies #946

Merged
merged 1 commit into from
Aug 21, 2020

Conversation

joan38
Copy link
Collaborator

@joan38 joan38 commented Aug 8, 2020

No description provided.

@joan38 joan38 force-pushed the bsp-build-sources branch from 046b921 to a52d9c4 Compare August 9, 2020 05:35
@joan38 joan38 force-pushed the bsp-build-sources branch from a52d9c4 to d75b3d6 Compare August 11, 2020 17:54
@joan38
Copy link
Collaborator Author

joan38 commented Aug 11, 2020

Ping @lefou @lihaoyi

@@ -132,24 +132,28 @@ class MillBuildServer(evaluator: Evaluator, bspVersion: String, serverVersion: S
val millBuildTargetId = getMillBuildTargetId(evaluator)

val items = dependencySourcesParams.getTargets.asScala
.filter(_ != millBuildTargetId)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We used to not resolve source dependencies for the Mill build dependencies. Which made code navigation hard since all we could see were the signature only.

Comment on lines +137 to +140
Try(getClass.getClassLoader.asInstanceOf[SpecialClassLoader]).fold(
_ => Seq.empty,
_.allJars.filter(url => isSourceJar(url) && exists(Path(url.getFile))).map(_.toURI.toString)
)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Now we get the source files for the build dependencies filtering with isSourceJar.

.fold(_ => Seq.empty, _.allJars.filter(url => exists(Path(url.getFile))).map(_.toURI.toString))
val classpath = Try(getClass.getClassLoader.asInstanceOf[SpecialClassLoader]).fold(
_ => Seq.empty,
_.allJars.filter(url => !isSourceJar(url) && exists(Path(url.getFile))).map(_.toURI.toString)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We are filtering out the source jars here since they are loaded in buildTargetDependencySources.

@lefou
Copy link
Member

lefou commented Aug 21, 2020

Can you please describe what the issue is and how this PR solves it.

@joan38
Copy link
Collaborator Author

joan38 commented Aug 21, 2020

Sure. In my build.sc if I want to know what transitiveIvyDeps does I will typically jump to definition (cmd+click) and see the implementation to answer most of my questions. This is what I did to answer this question on Gitter:

  /**
    * The transitive ivy dependencies of this module and all it's upstream modules
    */
  def transitiveIvyDeps: T[Agg[Dep]] = T{
    ivyDeps() ++ T.traverse(moduleDeps)(_.transitiveIvyDeps)().flatten
  }

But with the current state of BSP it looks like this:

  @mill.moduledefs.Scaladoc(value = """/**
    * The transitive ivy dependencies of this module and all it's upstream modules
    */""")
  def transitiveIvyDeps : mill.T[mill.api.Loose.Agg[mill.scalalib.Dep]] = { /* compiled code */ }

Because I don't have the sources loaded in my IDE, so the IDE decompiled the class files.

With this change, it loads the source jars of mill build dependencies accordingly.

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.

Looks good to me.

@lefou lefou merged commit d359a96 into com-lihaoyi:master Aug 21, 2020
@joan38 joan38 deleted the bsp-build-sources branch August 21, 2020 08:10
@lefou lefou added this to the after 0.8.0 milestone Aug 21, 2020
@joan38
Copy link
Collaborator Author

joan38 commented Aug 21, 2020

Thanks @lefou

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