Improvements for ingesting local JARs

[residentsummer]

• use it for building CP (Local JARs and classpath #273)
• use artifact version from analysis result instead of pom's one

[residentsummer]

• fixed analysis for remote jars (broken in first commits of this PR)
• split jar preparation out of analysis-impl
• added test for local jars

[martinklepsch]

Hey Anton!

Thanks a lot for your work on this, the jar approach is indeed much cleaner and it seems to be working well (I checked out your branch and tested things locally).

I added a few comments here and there mostly minor/informational stuff that you can take or leave however you feel.

The biggest point is probably around prepare-jar-for-analysis! and how this is weaved together with classpath construction and the rest of analyze-impl. Let me know what you think about my comments on this (I hope they make sense).

Thanks again 🙌 🙂

[martinklepsch]

I like how you pulled all that io stuff out of analyze-impl but I'm not sure if I think that the jar preparation work and class path calculation should be combined like this. An alternative idea:

• prepare-jar-for-analysis! returns {:jar-contents some-dir :jar-path local-jar-path}
• resolved-and-cp is still called very early in the process (similar place like before)
• this would require prepare-jar-for-analysis! to also be called earlier

I think right now we have a call stack that is

analyze-impl -> prepare-jar-for-analysis! -> resolved-and-cp

For testing in a REPL and also readability it could be nicer to have something more along the lines of this

(let [{:keys [jar-contents jar-path]} (prepare-jar-for-analysis! ,,,)
      {:keys [classpath]} (deps/resolved-and-cp jar-path ,,,)]
  (analyze-impl jar-contents classpath ,,,))

Let me know what you think :)

[residentsummer]

Hi, Martin!

Now I see that chaining functions like this will impede debugging in REPL. Sure, it's possible to rearrange calls to remove coupling and see the bigger picture. I got more questions, though :)

• I see that downloading and unpacking project JAR over and over again in analyze-impl (via prepare-jar...) will slow down debugging flow and obstruct the view on the process. But what are benefits of calculating CP separately from jar preparation?
• One point that bugged me in my current design is that prepare-... and analyze-impl are tightly coupled (prepare copies analysis files that will make sense only for analyze-impl). That's one of the reasons why call graph looks like this (the other reason is removing duplicate code from -main of runner and runner-ng). Do you think it will be better to put copying those files back to analysis-impl?
• I got some hard time with temp dir management in this PR... Moving prepare-jar-... out of analyze-impl, will require some kind of conveying a shared temp dir to both of them. I've already done this for prepare, but it still feels kinda clumsy...

[martinklepsch]

But what are benefits of calculating CP separately from jar preparation?

Sometimes when debugging dependency resolution issues I download .pom files from Clojars and point the existing code at those files. I imagine something similar might come in handy once this lands. I don't have strong feelings about repeated downloading etc.

One point that bugged me in my current design is that prepare-... and analyze-impl are tightly coupled

I'm not sure if there's a solution to this but maybe that's fine? After all the functions depend on another to get the job done.

I got some hard time with temp dir management in this PR...

I actually missed that fact during my review and that of course makes things a little more complicated. The tmp-dir creation could be moved outside the analyze-impl prepare- functions as well but then again this adds more arguments to those functions. Could be nice to pass a local directory though, maybe a bit easier to find than system temp dirs.

[martinklepsch]

I think this change makes sense but we should also document it in the cljdoc.cli docs. Something like "The version you specify via the CLI will take precedence over the version specified in the POM file".

[residentsummer]

Done. Please check if changes I've made to ingest command doc are ok :)

[martinklepsch]

But what are benefits of calculating CP separately from jar preparation?

Sometimes when debugging dependency resolution issues I download .pom files from Clojars and point the existing code at those files. I imagine something similar might come in handy once this lands. I don't have strong feelings about repeated downloading etc.

One point that bugged me in my current design is that prepare-... and analyze-impl are tightly coupled

I'm not sure if there's a solution to this but maybe that's fine? After all the functions depend on another to get the job done.

I got some hard time with temp dir management in this PR...

I actually missed that fact during my review and that of course makes things a little more complicated. The tmp-dir creation could be moved outside the analyze-impl prepare- functions as well but then again this adds more arguments to those functions. Could be nice to pass a local directory though, maybe a bit easier to find than system temp dirs.

[martinklepsch]

Honestly I'm perfectly fine with the way it is and just saw some potential to make things a bit more apparent for people who look at this code for the first time. At this point there's no requested changes anymore so feel free to do as much or as little as you wish and let me know when you're done and we'll get things merged :)

[residentsummer]

I've moved the call to prepare-jar... out of analyze-impl and packed the whole analysis flow into it's own function. Also removed redundant copying of analysis result file. There are changes for repl-based debugging as well - exposed some internal paths in the result of prepare-jar-... to make it possible to construct customized CP if needed.

let me know when you're done and we'll get things merged :)

Martin, I think I'm done with it :)

[martinklepsch]

Thanks @residentsummer! 🙌