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

Add support for lein 2.7's :managed-dependencies #454

Merged
merged 1 commit into from
Nov 25, 2016

Conversation

cprice404
Copy link
Contributor

In lein 2.7.0, a new feature was added that allows you to specify dependency versions in a new :managed-dependencies vector, so that you can, e.g., inherit dependency versions from a parent project (via the lein-parent plugin, etc.).

For more info see the Managed Dependencies docs in the lein repo.

The current release of lein-cljsbuild won't work with projects that use this, because the version checks at startup assume that they will be able to find the dependency version numbers directly in the :dependencies vector.

This commit adds support for getting the dependency versions via the new mechanism. It should be 100% backward compatible but it does introduce a min-lein-version of 2.7.

Opening this to solicit feedback. If there's another way to structure this in order to deal with the min-lein-version issue, let me know and I'll be happy to re-work it.

@@ -71,13 +72,18 @@
(map (fn [[k v]] (vec (cons k v)))))))

(defn make-subproject [project crossover-path builds]
(with-meta
(merge
(let [deps (classpath/merge-versions-from-managed-coords
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you check if this var exists before calling it? If it does then use it, if not then fall back to the old behaviour?

@cprice404 cprice404 force-pushed the lein-2.7-compat branch 2 times, most recently from efc60a2 to 2f55f15 Compare November 2, 2016 16:05
@cprice404
Copy link
Contributor Author

@danielcompton sure, I've updated the PR based on your suggestion.

I'm still in the process of verifying these changes against a local clojurescript project that uses :managed-dependencies - will add another comment here when I've confirmed it's working properly. Just wanted to get this up for some early review and see if it seemed like a generally acceptable patch.

@mneise
Copy link
Collaborator

mneise commented Nov 3, 2016

@cprice404 This looks good so far, would be happy to merge this 👍 Let me know when you think it's ready.

@cprice404
Copy link
Contributor Author

Thanks! I got sidetracked today so didn't finish my testing, hope to finish it tomorrow.

@cprice404
Copy link
Contributor Author

OK, done testing, confirmed that these changes work fine with my project, which uses :managed-dependencies heavily.

@mneise
Copy link
Collaborator

mneise commented Nov 3, 2016

@cprice404 I tested your patch with Leiningen 2.6.1 and get the following exception:

clojure.lang.Compiler$CompilerException: java.lang.RuntimeException: No such var: classpath/merge-versions-from-managed-coords, compiling:(leiningen/cljsbuild/subproject.clj:78:16)

I get the exception because it's still trying to compile the then branch and then can't find the var. To make it work you need to write a macro where you check for the var and return the code that should be used to get the dependencies 😉

@cprice404
Copy link
Contributor Author

Well, darn.

I could write a macro or I change the if to an if-let and then call the function via the resolved variable. The latter seems less messy to me but I'm fine doing it either way, so if you prefer the macro let me know.

@mneise
Copy link
Collaborator

mneise commented Nov 3, 2016

I don't have a preference, I think both approaches are ok. If you prefer the if-let approach, then go for it 😉

@cprice404
Copy link
Contributor Author

cool, working on it now. and testing with lein 2.6 this time :)

@cprice404
Copy link
Contributor Author

pushed up that change.

@cprice404
Copy link
Contributor Author

@mneise does this approach look OK?

@mneise mneise merged commit a268181 into emezeske:master Nov 25, 2016
@mneise
Copy link
Collaborator

mneise commented Nov 25, 2016

Thank you for this 👍

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.

3 participants