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

[java] Download 3rd-party Java sources from Maven #310

Merged
merged 2 commits into from
Jan 10, 2025

Conversation

alexander-yakushev
Copy link
Member

@alexander-yakushev alexander-yakushev commented Jan 5, 2025

Finally, a way to download source JARs at runtime. Sister PRs: clojure-emacs/cider-nrepl#911, clojure-emacs/cider#3769.

I tried a few approaches and settled with this one – downloading happens within class->source-file-url if *download-sources-jar-fn* is bound. The caller is expected to bind it to something that eventually invokes download-sources-jar-for-coordinates but can do custom logic and predicates too. I'm not 100% happy with this solution, it feels kinda awkward, but this is the best I could do. I honestly tried to make it strictly "cider-nrepl invokes Orchard", but the amount of repeated work was too much. Common Lisp condition system would actually make this cleaner, but hey, that's what dynamic vars actually are in the end, right?

Different ways of downloading is supported. Ultimately, all of them involve spawning subprocesses:

  • Clojure 1.12 projects with clojure build tool can invoke the new shiny invoke-tool function.
  • Pre-1.12 you can subshell clojure directly.
  • Leiningen projects can call lein.

Orchard doesn't do build tool detection on its own, the caller is expected to pass it explicitly.


  • You've added tests to cover your change(s)
  • You've updated the changelog

@bbatsov
Copy link
Member

bbatsov commented Jan 5, 2025

I'm not 100% happy with this solution, it feels kinda awkward, but this is the best I could do. I honestly tried to make it strictly "cider-nrepl invokes Orchard", but the amount of repeated work was too much. Common Lisp condition system would actually make this cleaner, but hey, that's what dynamic vars actually are in the end, right?

Yeah, it's not work of art, but I think the current implementation is fine, and we can always polish it down the road as I doubt it will have other users besides cider-nrepl for the foreseeable future.

Perhaps it'd be good to add some section to the README explaining how clients are supposed to use *download-sources-jar-fn*.

@alexander-yakushev alexander-yakushev force-pushed the download-sources branch 3 times, most recently from cb67964 to 3369f4c Compare January 5, 2025 22:56
@alexander-yakushev
Copy link
Member Author

alexander-yakushev commented Jan 5, 2025

Expanded the README.

Also, moved the discovery of the underlying build tool here. It is more reliable to perform on the Clojure side.

I don't rush to merge this; I will start merging once I get approval on all three PRs.

@alexander-yakushev alexander-yakushev force-pushed the download-sources branch 2 times, most recently from d0c9f80 to 99544f1 Compare January 5, 2025 23:06
Only cache parsed classes where the source was present. This gives a chance to
trigger the sources download if it was missing after a non-downloading calls to
class-info were made (e.g. by "eldoc" op).
@alexander-yakushev alexander-yakushev merged commit 86af29e into master Jan 10, 2025
19 checks passed
@alexander-yakushev alexander-yakushev deleted the download-sources branch January 10, 2025 09:24
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