make bloop aware of runtime dependencies

[megri]

At the moment, mill generates bloop configurations without setting the runtime classpath of Java modules. This makes bloop fall back to project.classpath which corresponds with required compile-time classpath. It makes sense to put stuff like slf4j-adapters, postgres-drivers and such—dependencies that are only required during runtime—in the JVM-configuration.

The following discussion on the bloop issue tracker highlights the requirements and the accepted solution: scalacenter/bloop#1233
scalacenter/bloop#1244

This pull-request simply outputs the runClasspath target to BloopConfig.Platform.Jvm#classpath. I'm not a 100 % sure this is the correct approach, but it corresponds to what gets put on the classpath when having mill run a project (as far as I can tell, from looking at mill inspect app.run.

[megri]

I need to change my approach: the generated bloop config references classes in the mill out path. This had me very confused as changes to my code didn't register properly.

[lefou]

I have two concerns:

First, I think the transitive dependency calculation isn't correct, because it includes also compile-time dependencies transitively, which is wrong. I also realize this issue was already present before your PR, but as you touched this code, I wanted to point it out. And at this point, I'd like to request a test case resembling a project with multiple modules and ensuring the classpaths and run-classpaths are correct, which brings me to the other point.

This is a contrib module, where we don't enforce the same strict quality standards. I guess this means, we can just merge as is, with missing tests and such. But as Bloop is used by many users (I guess), we should address my first concern.

[lefou]

Oh, and thanks for creating this PR. Totally forget that, as I were so concentrated on the implementation.

[lefou]

I wrote the classpaths down how I think they should be used by bloop. But please beware that I haven't tested them.

    val compileClasspath = T.task {
      val depModules = (module.compileModuleDeps ++ module.recursiveModuleDeps).distinct
      // dep modules ++ ivy deps ++ unmanaged
      depModules.map(classes) ++
        module.resolvedIvyDeps().map(_.path) ++
        module.unmanagedClasspath().map(_.path)
    }

    val runtimeClasspath = T.task {
      // dep modules ++ ivy deps ++ unmanaged
      module.recursiveModuleDeps.map(classes) ++
        module.resolvedRunIvyDeps().map(_.path) ++
        module.unmanagedClasspath().map(_.path)
    }

I don't know how bloop is handling resources, so I completely ignored that part.

[megri]

I wrote the classpaths down how I think they should be used by bloop. But please beware that I haven't tested them.

    val compileClasspath = T.task {
      val depModules = (module.compileModuleDeps ++ module.recursiveModuleDeps).distinct
      // dep modules ++ ivy deps ++ unmanaged
      depModules.map(classes) ++
        module.resolvedIvyDeps().map(_.path) ++
        module.unmanagedClasspath().map(_.path)
    }

    val runtimeClasspath = T.task {
      // dep modules ++ ivy deps ++ unmanaged
      module.recursiveModuleDeps.map(classes) ++
        module.resolvedRunIvyDeps().map(_.path) ++
        module.unmanagedClasspath().map(_.path)
    }

I don't know how bloop is handling resources, so I completely ignored that part.

I tried recompiling mill with these changes and the generated bloop config seemingly works for my setup! That is, postgres driver and slf4j-adapter is made available only during runtime.

[lefou]

Great. Do you intend to add a test case, too?

If not, maybe you could share your build.sc, so I can try to make an realistic test case of it.

[megri]

Great. Do you intend to add a test case, too?

If not, maybe you could share your build.sc, so I can try to make an realistic test case of it.

I can have a look at it!

How do you imagine the test should look like? A simple build definition, running bloopimpl and then comparing the output to a predefined expected result, or something more involved?

[lefou]

I can have a look at it!

How do you imagine the test should look like? A simple build definition, running bloopimpl and then comparing the output to a predefined expected result, or something more involved?

Great. Yes, just comparing the generated output looks good to me. We do something similar in GenIdeaTests, where we also have some rudimentary support to ignore some parts of the generated files to work around variables parts. Don't know if that is necessary.

[megri]

I can have a look at it!
How do you imagine the test should look like? A simple build definition, running bloopimpl and then comparing the output to a predefined expected result, or something more involved?

Great. Yes, just comparing the generated output looks good to me. We do something similar in GenIdeaTests, where we also have some rudimentary support to ignore some parts of the generated files to work around variables parts. Don't know if that is necessary.

I amended the existing tests in BloopTests.scala. I think I got it working, and also added a check for compile-time dependencies while at it.

I couldn't figure out how to get access to the resolved ivy deps so there's some extra in-place filtering done in the assertions to make sure that all deps except compileIvyDeps are present in the jvm platform classpath. I'm fine with cleaning this up if there's a better way of doing things!

[megri]

The failing tests seem to be related to something else..

[lefou]

The failing tests seem to be related to something else..

Yeah, I'm already trying to fix them. This PR looks good to me now. Let's rebase/merge on main branch once I've pushed my test changes and if we are green, we can merge.

[megri]

The failing tests seem to be related to something else..

Yeah, I'm already trying to fix them. This PR looks good to me now. Let's rebase/merge on main branch once I've pushed my test changes and if we are green, we can merge.

Sounds good!

[lefou]

@megri Can you rebase this PR on latest main branch, please?

[megri]

Done!

[lefou]

Thank you!