Restrict or remove usages of add-classpath!

[vemv]

Context

There's a number of issues indicating that the use of orchard.java/add-classpath! creates issues that end users can experience:

#112
#103
#105

Some time has passed since those were reported (especially for clojure-emacs/cider-nrepl#668 - more than a year) so it seems a good idea to take action.

Proposal

Any of these would seem sensible:

• Make features/dependencies that ultimately use dynapath/add-classpath! opt-out
  • Pro: no changes needed for current consumers, wanting to use related features
  • Con: introduces the need to convey to users that they likely want to set this property
  • Con: by default, the same currently-buggy behavior would arise.
• Make features/dependencies that ultimately use dynapath/add-classpath! opt-in
  • Pro: bug-free experience by default
• Remove dynapath/add-classpath! entirely.
  • Pro: bug-free experience by default
  • Con: perhaps an excessively dramatic measure, that would make Git history more churn-y, assuming that in a later moment a fix is found for these issues.
    • Can also invite clients to create their own solutions, which is kind of an anti-goal for Orchard, per its README

How

For the two first alternatives:

• Make [org.tcrawley/dynapath "1.1.0"] a :provided dependency, and within orchard, only require it dynamically via a try/catch
• Guard all usages of add-classpath! under a "feature flag"
  • namely, a system property akin to https://github.com/clojure-emacs/orchard/tree/a233ee6867ee30f769c9a0526cca10efb1508cb6#configuration-options
  • the default value for the system property would depend on whether "opt-in" or "opt-out" is preferred.

Other considerations

It's worth reminding that add-classpath! has essentially (afaict) two big use cases:

• making sources and javadocs available
• adding dependencies in runtime

For the former, that is a problem to which I happen to have found a robust solution, free of classloader intricacies. See clojure-emacs/cider-nrepl#64 (comment)

This is to say, for 50% of the use cases there's a real alternative that can be used today. The other 50% is not unimportant, but likely it doesn't justify introducing buggy behavior that can affect unrelated 3rd-party libraries.

[bbatsov]

It's worth reminding that add-classpath! has essentially (afaict) two big use cases:

Indeed. And we don't really use this for adding depedencies at runtime, so you can argue we've got just one use-case in practice. On top of this - it seems our use case causes problems for many people, as you've already noted, that's why I'm leaning more and more towards:

1. Moving the classpath modification stuff to cider-nrepl, where in theory it should be simpler as at least you can operate directly on the session classloaders
2. Removing this completely if you manage to extend your Lein plugin approach to the JDK's own sources and JavaDocs.

Can also invite clients to create their own solutions, which is kind of an anti-goal for Orchard, per its README

Yeah, that's true - but if our solution doesn't work I guess we're still inviting them to do something else. ;-) There's also the part about Orchard aiming to have 0 runtime deps and dynapath being the only one. In a way, that's not a big deal, as it's unlikely that the user apps depend on dynapath, but I'd still love to get to a self-contained library at some point.

[bbatsov]

Some time has passed since those were reported (especially for clojure-emacs/cider-nrepl#668 - more than a year) so it seems a good idea to take action.

Yeah, that's regrettable, but it's also a reflection for how little time we've all been able to dedicate to Orchard and related libraries recently. :-( This doesn't change the fact that it's probably one of the most important problems to address, given how many people are affected by it.

[vemv]

Thanks for the thoughts!

SGTM. I can provide a PR removing dynapath entirely, if that helps.

Also good to hear that the Lein plugin approach would be welcome as well. I was waiting to see how theseclojure-emacs issues would develop before implementing further features (JDK sources, tools.deps compat).

They are a stone's throw away so it's safe to assume they'll be available in a couple weeks :)

[bbatsov]

Let’s do this in two steps. You can first make this opt-in, I’ll cut a new release and then we’ll remove it completely 1-2 more releases down the road. I think this will make the transition less painful for people who are okay with the current approach. I don’t want to remove this completely without having an alternative we can suggest. Not that the current approach works for me. 😂It would also be great to add some instructions about your Lein plugin to CIDER’s docs.

On Sun, 11 Apr 2021 at 9:44, vemv ***@***.***> wrote: Thanks for the thoughts! SGTM. I can provide a PR removing dynapath entirely, if that helps. Also good to hear that the Lein plugin approach would be welcome as well. I was waiting to see how theseclojure-emacs issues would develop before implementing further features (JDK sources, tools.deps compat). They are a stone's throw away so it's safe to assume they'll be available in a couple weeks :) — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#113 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAZLSWT7JHEHK4SKYST7T3TIFAMBANCNFSM42XIYWLQ> .

-- Best Regards, Bozhidar Batsov

[vemv]

👍 Perfect! Doing