search

boot-clj / boot

Build tooling for Clojure.
https://boot-clj.github.io/
Eclipse Public License 1.0
1.75k stars 179 forks source link

Better warning when there is a classpath conflict for org.clojure/clojure #672

Open martinklepsch opened 6 years ago

martinklepsch commented 6 years ago 
Classpath conflict: org.clojure/clojure version 1.8.0 already loaded, NOT loading version 1.3.0

If you specify a Clojure version in boot.properties and don't specify the same Clojure version in your :dependencies you will get a warning like the one above.

Not sure if this is worth it but we could provide some more context for the specific case of Clojure:

Classpath conflict: org.clojure/clojure version 1.8.0 already loaded, NOT loading version 1.3.0
This is commonly caused by:
- not specifying a Clojure version in your :dependencies
- specifying a different version of Clojure than in boot.properties

What do you think?

mprokopov commented 6 years ago

An another workaround is to make exclusions in dependencies, as I've described here https://stackoverflow.com/questions/43018876/classpath-conflict-org-clojure-clojure-version-1-7-0-already-loaded-not-loadin/48282148#48282148 But it's better to resolve this warning by inclusion of proper version of Clojure.

And in case you forget to include Clojure in app dependency list, you may end up with the bug, when uber jar is compiled successfully, but application can not find its entry point when launched. I spend almost a day figuring out this bug. Hope it helps someone.

martinklepsch commented 6 years ago

@mprokopov Would you be interested in providing a PR for this? 🙂

mprokopov commented 6 years ago

@martinklepsch I think this is good idea to provide some hints in warning message to resolve this issue for an end user, but this warning could easily be other than org.clojure/clojure.

Inclusion of dependency in top dependency tree seems to be proper resolution for any library with such warning. Is it reasonable to advice user to include dependency in a such way?

Maybe it's just worth to be noted in README/FAQ for boot.

martinklepsch commented 6 years ago

I think org.clojure/clojure is a special case here because you need to specify it in two places: build.boot and boot.properties. This does not apply to libraries where this warning usually only appears if you call set-env! multiple times and dependencies get resolved differently.

While having multiple set-env! calls is usually something you want to avoid it also is handy in a REPL/exploratory context.

Also note that the code checks for Clojure specifically and then warns "NOT loading" whereas with regular libraries it warns "ALSO loading: https://github.com/boot-clj/boot/blob/cb923ee78ada4ade6ec60782f4b3b1762325ce30/boot/core/src/boot/core.clj#L240-L248

Besides a warning we could also just add the version of Clojure to the list of :dependencies but this would most likely be a breaking change.

So — following suggestion:

  1. We create a PR for this issue that takes care of warning about the special case of org.clojure/clojure
  2. We open an additional issue to figure out what to do with the general case of multiple set-env! calls. I think the warning and a link to a document with some context around why it should only be done for exploratory stuff could be a good way forward but this is up for discussion and should be treated separately from this issue which focuses on the Clojure dependency specifically.

Does that sound good to you?

martinklepsch commented 6 years ago

@mprokopov hey again! Just wanted to check what you think about my suggestion and if you’d be interested to help with this? 🙂

mprokopov commented 6 years ago

Hi again!

I think it's better just to make this warning a bit better with explanation you suggested:

Classpath conflict: org.clojure/clojure version 1.8.0 already loaded, NOT loading version 1.3.0
This is commonly caused by:
- not specifying a Clojure version in your :dependencies
- specifying a different version of Clojure than in boot.properties

But it would be not so good, if this full warning message will be displayed several times. For example I forgot to include top Clojure dependency, and ring, ring-jetty required Clojure 1.6.3, and pandeiro/boot-http Clojure 1.7.0, so I've got 3 warnings about it.

How do you think @martinklepsch, is it enough just to update boot wiki FAQ with your suggestion?

martinklepsch commented 6 years ago

For example I forgot to include top Clojure dependency, and ring, ring-jetty required Clojure 1.6.3, and pandeiro/boot-http Clojure 1.7.0, so I've got 3 warnings about it.

The warning will only get printed three times if you call set-env! with each of these dependencies individually. This arguably happens less often but it could still become annoying. Not sure what to do — does this change your opinion?

arichiardi commented 6 years ago

I would also mention that org.clojure can be put in :exclusions automatically by using the -x option to boot.

burn2delete commented 5 years ago

I agree with @martinklepsch , shadow-cljs is patching their dependencies and warning the user that setting the clojure version has no effect and to remove it. I think this is the correct way to reason about it. This prevents the user from creating a conflict to begin with.

We could do the same, patch the version in the dependencies with whats in boot.properties and notify the user, again breaking change.