scalameta / mdoc

Typechecked markdown documentation for Scala
https://scalameta.org/mdoc/
Apache License 2.0
396 stars 81 forks source link

Wrong typesafe config on classpath when evaluating #481

Open tgodzik opened 3 years ago

tgodzik commented 3 years ago

It seems that somehow when using for example akka in worksheets the actual typesafe config lib that is being loaded is coming from mdoc, not from akka. Which means that akka will not work and throw ClassNotFound exceptions since mdoc has 1.2.1 version on the classpath. I have currently no idea how to fix it (we should be using the correct classpath currently), so I decided to make the classpath a bit thiner in Metals as worksheets do not need everything that mdoc brings. Especially, as there are no config options, so no need for the typesafe config.

ckipp01 commented 3 years ago

So I dove into this today hoping to get a better understanding and maybe figure this out. I think I left more confused, and just with questions. 😆

One thing I noticed that doesn't make a ton of sense to me is that shouldn't the class path here also only contain the same subset of paths that makeup runtime below? Why is the full defaultClasspath kept if no class path is give, but if a class path is give, then we only keep the absolutely necessary stuff for runtime. Couldn't we also keep the thinned out paths if the given class path is empty?

https://github.com/scalameta/mdoc/blob/900cc5b3d3fb953e501bd7170f76a124ad7752aa/mdoc/src/main/scala/mdoc/internal/markdown/MarkdownBuilder.scala#L80-L98

Let's say that we do the above change and make the class path a bit thinner that MarkdownCompiler has. Even then inside of MarkdownCompiler we create the appClassLoader that has that class path and also this.getClass.getClassLoader.

https://github.com/scalameta/mdoc/blob/900cc5b3d3fb953e501bd7170f76a124ad7752aa/mdoc/src/main/scala-2/mdoc/internal/markdown/MarkdownCompiler.scala#L60-L62

This is used when we create the AbstractFileClassLoader, which is what we use to create the DocumentBuilder and ultimately the class loader that is used to in runInClassLoader.

https://github.com/scalameta/mdoc/blob/900cc5b3d3fb953e501bd7170f76a124ad7752aa/mdoc/src/main/scala-2/mdoc/internal/markdown/MarkdownCompiler.scala#L129

https://github.com/scalameta/mdoc/blob/900cc5b3d3fb953e501bd7170f76a124ad7752aa/mdoc/src/main/scala/mdoc/internal/markdown/MarkdownBuilder.scala#L45

I tried playing around a bunch with switching the classloader here, since I thought just using a different class loader for when we load up the appClassLoader would work, but it doesn't since then the repl.MdocSession$ and the DocumentBuilder we try to cast to end up being in different classloaders, and that doesn't work. My assumption was and sort of still is that the issue lies here:

https://github.com/scalameta/mdoc/blob/900cc5b3d3fb953e501bd7170f76a124ad7752aa/mdoc/src/main/scala-2/mdoc/internal/markdown/MarkdownCompiler.scala#L60-L62

See that we are using the current classloader, but I don't know how to get around that.

tgodzik commented 3 years ago

I tried playing around a bunch with switching the classloader here, since I thought just using a different class loader for when we load up the appClassLoader would work, but it doesn't since then the repl.MdocSession$ and the DocumentBuilder we try to cast to end up being in different classloaders, and that doesn't work. My assumption was and sort of still is that the issue lies here:

https://github.com/scalameta/mdoc/blob/900cc5b3d3fb953e501bd7170f76a124ad7752aa/mdoc/src/main/scala-2/mdoc/internal/markdown/MarkdownCompiler.scala#L60-L62

See that we are using the current classloader, but I don't know how to get around that.

That seems to be the main problem. We need it to be on the same classloader and on the other hand we don't really want that :/ I also played around it (should have mentioned it in the issue, sorry!) and haven't come up with any short term solution. Maybe we could use system classloader plus java interfaces trick as we do in Metals? Not sure if that would also work.