search

benedekfazekas / mranderson

Dependency inlining and shadowing
Eclipse Public License 1.0
151 stars 14 forks source link

Fix #44: Duplicated namespaces (e.g. in riddley) #45

Closed xsc closed 3 years ago

xsc commented 3 years ago

This is my attempt at fixing #44, which I ran into when trying to use mranderson with lein-ancient. To preserve backwards-compatibility as far as possible, I elected to only act if there was more than one file in a package that resolves to the same namespace. Additional consideration was necessary to not accidentally remove files if there were both CLJ/CLJS sources in the package.

All in all, the effect might be very similar to that of ba9154cc315533c0170dc44a36c344d4155593f6, just makes the logic a bit more explicit and moves it to a higher level instead of having it deep inside update-artifact!.

This PR also includes a fix for replacing namespace symbols in a data_readers.cljc. Currently, mranderson chokes on this file because it expects all CLJC files to have a namespace declaration. I think we should be more forgiving here.

benedekfazekas commented 3 years ago

this looks really good, thank you.

Just wondering if you could add a test for the NPE fix (there is a test ns for mranderson.move so that should not be too much pain). can't say the same about the riddley fix so that is fine wo/ a test ;)

ta, really nice work!

xsc commented 3 years ago

@benedekfazekas I added a testcase for the NPE fix. I also made the deduplication more explicit in 8e3111d (see commit message) to avoid an edge case.

benedekfazekas commented 3 years ago

sounds awesome. will have a look ASAP

benedekfazekas commented 3 years ago

thanks for the test, LGTM.

tested this with cider-nrepl before merging and there seems to be a problem.

when trying to run make inline-deps on current master/HEAD of cider-nrepl with a locally deployed version of the code in this PR I get

fazekasbenedek@scatha:~/projects/cider-nrepl(master)$ make inline-deps
lein inline-deps
project prefix:  cider.nrepl.inlined-deps
retrieve dependencies and munge clojure source files
in RESOLVED-TREE mode, working on a resolved dependency tree
 [org.clojure/tools.namespace "1.0.0" :exclusions [[org.clojure/clojure]]]
   [org.clojure/java.classpath "1.0.0"]
 [compliment "0.3.11" :exclusions [[org.clojure/clojure]]]
 [org.clojure/tools.trace "0.7.10" :exclusions [[org.clojure/clojure]]]
 [org.rksm/suitable "0.3.5" :exclusions [[org.clojure/clojure] [org.clojure/clojurescript]]]
 [org.clojure/tools.reader "1.3.2" :exclusions [[org.clojure/clojure]]]
 [cljfmt "0.7.0" :exclusions [[org.clojure/clojure] [org.clojure/clojurescript]]]
   [com.googlecode.java-diff-utils/diffutils "1.3.0"]
   [org.clojure/tools.cli "1.0.194"]
   [rewrite-clj "0.6.1"]
   [rewrite-cljs "0.4.5"]
 [cider/orchard "0.6.1" :exclusions [[org.clojure/clojure]]]
   [org.tcrawley/dynapath "1.1.0" :exclusions [[org.clojure/clojure]]]
 [thunknyc/profile "0.5.2" :exclusions [[org.clojure/clojure]]]
 [mvxcvi/puget "1.3.1" :exclusions [[org.clojure/clojure]]]
   [mvxcvi/arrangement "1.2.0"]
 [fipp "0.6.23" :exclusions [[org.clojure/clojure]]]
   [org.clojure/core.rrb-vector "0.1.1"]
unzipping [ cljfmt  [ v0v7v0 ]]
java.lang.NullPointerException: null
 at clojure.core$name.invokeStatic (core.clj:1595)
    clojure.core$name.invoke (core.clj:1589)
    mranderson.util$sym__GT_file_name.invokeStatic (util.clj:32)
    mranderson.util$sym__GT_file_name.invoke (util.clj:30)
    mranderson.core$remove_invalid_duplicates_BANG_$fn__5466.invoke (core.clj:261)
    clojure.core$map$fn__5851.invoke (core.clj:2755)
    clojure.lang.LazySeq.sval (LazySeq.java:42)
    clojure.lang.LazySeq.seq (LazySeq.java:51)
    clojure.lang.RT.seq (RT.java:531)
    clojure.core$seq__5387.invokeStatic (core.clj:137)
    clojure.core$apply.invokeStatic (core.clj:660)

I suppose remove-invalid-duplicates! should not expect all files having an ns macro either. cljfmt has quite a few of those in its resources -- with .clj extension as well. Maybe we can skip trying to remove duplicates for these files -- they can't really be duplicates without an ns macro. we still want to unzip them though I think.

xsc commented 3 years ago

@benedekfazekas So, I got a bit worried about all of the potential edge cases, and decided to add an end-to-end test that simulates a run of mranderson (9c83beb), albeit with a restricted set of options.

That test failed because of the problem you discovered which let me provably fix it (and also prove that riddley now works). I'll still need to add a few more assertions (e.g. to verify that the dependency source files have been moved and the namespaces changed) but it's definitely already reviewable.

Update: Added 57ebd2b which checks one of the riddley source files.

benedekfazekas commented 3 years ago

sorry for pushing this into slightly bigger scope than you probably originally intended. will properly review ASAP

thanks again for the good work

xsc commented 3 years ago

@benedekfazekas No worries, and don't feel obliged to review stuff asap, I have time. :)

benedekfazekas commented 3 years ago

@xsc I am ready to merge this. grateful for the extra test, will have its uses outside the scope of this issue for sure :)

do you want to squash the commits and rewrite/merge the commit msgs yourself or are you happy if I use the github facility to squash?

xsc commented 3 years ago

@benedekfazekas Cleaned up the commits! Ready to go from my side! :)

benedekfazekas commented 3 years ago

merged. will deploy a snapshot version to clojars shortly. ta!

bbatsov commented 3 years ago

I think you can directly cut a new release, as the change seems fairly safe to me.