ClojureScript

When compiling incrementally, goog dependencies can get out of order (breaks builds)

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:

Description

In lein-cljsbuild version 0.2.0, I changed it to allow incremental compilation again. Previous versions would always clean out the :output-dir before each call to closure/build, because I had seen weird issues before but never tracked them down.

Finally, I have pinpointed the problem with incremental compilation (although not the solution). The problem is that sometimes, when the compiler notices that a ClojureScript file has already been compiled to JavaScript, its JavaScript output will be inserted into the :whitespace (or higher) optimized output in the wrong order. By "wrong order", I mean its goog.provide() comes after a goog.require() for that same namespace.

I've attached a snippet from my :whitespace compiled output for a closed source project. I removed a bunch of code from the attachment, but I did not change the order of anything. As you can see, ss_charts.crossover.url is provided after it is required.

My guess is that the problem originates in compiler/compile-file. It calls requires-compilation?, and returns a hash containing only {:file ...} if it does not require compilation. I assume that the problem has to do with the fact that this hash lacks the :provides and :requires keys, and thus down the road doesn't get considered in the dependency graph correctly.

Activity

Hide
David Nolen added a comment -

Seems like a reasonable guess. Patch welcome.

Show
David Nolen added a comment - Seems like a reasonable guess. Patch welcome.
Hide
Arlen Christian Mart Cuss added a comment -

There's a minimalish reproducible example of this here: https://github.com/unnali/cljs-sscce.

Show
Arlen Christian Mart Cuss added a comment - There's a minimalish reproducible example of this here: https://github.com/unnali/cljs-sscce.
Hide
David Nolen added a comment -

I'm unable to recreate the issue myself when using the example project. I've created a patch that may fix the issue for others. Can someone please confirm? You can test the patch by making a checkouts directory, clone ClojureScript into it, apply the patch there and then add the following to your project.clj:

:extra-classpath-dirs ["checkouts/clojurescript/src/clj"
                       "checkouts/clojurescript/src/cljs"]
Show
David Nolen added a comment - I'm unable to recreate the issue myself when using the example project. I've created a patch that may fix the issue for others. Can someone please confirm? You can test the patch by making a checkouts directory, clone ClojureScript into it, apply the patch there and then add the following to your project.clj:
:extra-classpath-dirs ["checkouts/clojurescript/src/clj"
                       "checkouts/clojurescript/src/cljs"]
Hide
Arlen Christian Mart Cuss added a comment -

Unfortunately, this didn't fix the issue for me. I've just realised the cljs-sscce as linked had ":incremental false" set in project.clj, which stops the issue from being reproduced. If you didn't notice that (I didn't until I came to try and found it "working" pre-patch!), it might be worth trying again with that.

If you still can't reproduce it, let me know—I can try preparing a VM image or similar?

Show
Arlen Christian Mart Cuss added a comment - Unfortunately, this didn't fix the issue for me. I've just realised the cljs-sscce as linked had ":incremental false" set in project.clj, which stops the issue from being reproduced. If you didn't notice that (I didn't until I came to try and found it "working" pre-patch!), it might be worth trying again with that. If you still can't reproduce it, let me know—I can try preparing a VM image or similar?
Hide
David Nolen added a comment -

OK, I was able to reproduce the original issue. I can also confirm that the issue is fixed for me per the instructions on the cljs-sscce repo. I note that it's not really possible to confirm w/ my instructions in the comment above with Lein 2.

Show
David Nolen added a comment - OK, I was able to reproduce the original issue. I can also confirm that the issue is fixed for me per the instructions on the cljs-sscce repo. I note that it's not really possible to confirm w/ my instructions in the comment above with Lein 2.
Hide
Arlen Christian Mart Cuss added a comment -

My testing's with Lein 2, so as David noted the fact that it didn't work for me is no indication that the patch doesn't work!

Show
Arlen Christian Mart Cuss added a comment - My testing's with Lein 2, so as David noted the fact that it didn't work for me is no indication that the patch doesn't work!
Hide
David Nolen added a comment -

I believe you may be able to confirm the patch by skipping the checkouts bit, just clone clojurescript to some directory in your project and set :extra-classpath-dirs.

Show
David Nolen added a comment - I believe you may be able to confirm the patch by skipping the checkouts bit, just clone clojurescript to some directory in your project and set :extra-classpath-dirs.
Hide
David Nolen added a comment -

I checked with Phil Hagelberg, in Lein 2 instead of using :extra-classpath-dirs you need to use :resource-paths and no need to create the checkouts directory.

Show
David Nolen added a comment - I checked with Phil Hagelberg, in Lein 2 instead of using :extra-classpath-dirs you need to use :resource-paths and no need to create the checkouts directory.

People

Vote (4)
Watch (3)

Dates

  • Created:
    Updated:
    Resolved: