feat: add moduleDeps classes to the classpath #4
Conversation
|
|
||
| javaBuilder.addDependencies(libraryDeps.filter(x => os.exists(x.path)).map(x => x.path.wrapped).toList.asJava) | ||
| javaBuilder.addSnapshotDependencies(upstreamClassSnapShot.map(_.path.wrapped).toList.asJava) | ||
| javaBuilder.addToClasspath(moduleDeps.filter(x => os.isDir(path)).map(_.path.wrapped).toList.asJava) |
There was a problem hiding this comment.
| javaBuilder.addToClasspath(moduleDeps.filter(x => os.isDir(path)).map(_.path.wrapped).toList.asJava) | |
| javaBuilder.addToClasspath(moduleDeps.filter(x => os.isDir(x.path)).map(_.path.wrapped).toList.asJava) |
typo?
|
|
||
| javaBuilder.addDependencies(libraryDeps.filter(x => os.exists(x.path)).map(x => x.path.wrapped).toList.asJava) | ||
| javaBuilder.addSnapshotDependencies(upstreamClassSnapShot.map(_.path.wrapped).toList.asJava) | ||
| javaBuilder.addToClasspath(moduleDeps.filter(x => os.isDir(path)).map(_.path.wrapped).toList.asJava) |
There was a problem hiding this comment.
There's also javaBuilder.addProjectDependencies which accepts JARs, if they're available.
|
|
||
| javaBuilder.addDependencies(libraryDeps.filter(x => os.exists(x.path)).map(x => x.path.wrapped).toList.asJava) | ||
| javaBuilder.addSnapshotDependencies(upstreamClassSnapShot.map(_.path.wrapped).toList.asJava) | ||
| javaBuilder.addToClasspath(moduleDeps.filter(x => os.isDir(path)).map(_.path.wrapped).toList.asJava) |
There was a problem hiding this comment.
Note that addToClasspath expects individual files, not a directory.
| val (moduleDeps, libraryDeps) = | ||
| upstreamClass.partition(MDShared.isModuleDep(_)) | ||
|
|
||
| javaBuilder.addDependencies(libraryDeps.filter(x => os.exists(x.path)).map(x => x.path.wrapped).toList.asJava) |
There was a problem hiding this comment.
Also, why are there the filters? Aren't they just hiding potential bugs? All the files must exist. If not, something somewhere is broken, no?
There was a problem hiding this comment.
@jhenahan thanks for the PR - generally the multi module build is definitly far from ideal yet (hence also part of the future work in the readme).
When i pushed the first release I tried to port a pre-existing project libro-finito using the mill included docker module to this plugin -> and immediatly realized i havent covered multi module nicely.
Generally I would like to test / implement the multi module build in more detail - to catch more corner cases.
Is the repo you are using it in public? (didnt find it on first glance)
I have to admit right now I'm a bit swamped with Job Hunting/Interviews/Interview Prep (current Job Market sucks 🙈) - but once i have something signed i should be able to get back onto it and invest some time.
Just a heads up why things will not move much next month. I will ping / mention you when I'm back onto it
@sideeffffect
Thats by the way why there is an "exists" clause at the line you pointed out. You are absolutly right - its covering bugs - in particular in this multi module setup.
At the time it seemed to be issues with what mill reported for multi module builds and i did not want to get into the depth of it (as it ment digging deeper into the mill internals) and put it for "future work" 😅
|
Hello everybody, is there something I can help with to move this forward? |
|
Sorry, got bogged down in some other things and wasn't working with Mill for a bit there. Let me take a look at the comments and see if I can polish this up a bit and then we can get it landed. |
|
I finally can spend time on moving this plugin forward a bit - sorry for the long delay on my side. |
|
Yeah, I'll put together a little reproducer repo today. The original motivation is unfortunately an internal repo. |
|
@jhenahan @GeorgOfenbeck ping 😸 |
|
Hello @jhenahan @GeorgOfenbeck , do you have any idea when you will be continuing the work on this PR? |
|
Sorry, I rolled off the work project that motivated this in the first place (and haven't been on other mill projects in the interim), but I've got some bandwidth this week. I'll read back through the comments here and see if I can put something less completely hacky together to close this out. |
3 participants
Motivation
In my code, I am building images with this plugin, but I've had trouble getting the resulting images to run due to
ClassDefNotFounderrors on startup. Eventually it clicked that these were classes from mymoduleDeps, and sure enoughdiveshowedapp/classes.jar/was empty in the resulting images. I think this is just down to themoduleDepsending up as class files rather than JARs, soaddDependenciesjust ignores them.Result
I've added a fairly fragile heuristic to grab the
compile.dest/classespaths from theupstreamAssemblyClasspathpassed intoMDBuildand use them withaddToClasspath, which recursively copies the passed directories into/app/classpath/(preserving the directory structure) and adds/app/classpath/to the default entrypoint.It would probably be better to use
transitiveLocalClasspathsince it only containsmoduleDepsoutputs, but I figured I'd get this out sooner than later so I don't have to maintain my little workaround.Workaround
Until this is released, the same effect can be achieved with a line like
in an overridden
getJavaBuilder.