ClojureScript

Recompile dependents broken when files are not inside one directory

Details

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

Description

When a file is changed files dependent on that file are marked for recompile. This doesn't seem to work when the changed file is not in the directory which is passed to build and the directory where dependent namespace is.

Looks like this caused by how build and compile-file work:

  • Build calls -compile -> compile-dir -> compile-root which sorts the files in dependency order and calls -compile for the files in dependency order
    • core ns is compiled
  • Build calls add-depedencies -> add-dependencies -> get-compiled-cljs -> -compile -> compile-file
    • test ns is compiled, core ns is marked as dependent on test ns but as core ns is already compiled this doesn't matter

I believe this effectively breaks :recompile-dependents in cases all cases but one where all the sources are inside one directory. In multiple directory case order of directories passed to cljs.build.api/inputs affects the order on which compile is called so it's possible that some dependent namespaces are compiled before namespaces they depend on are compiled.

Original problem was that added tests were not picked up by run-tests but I have since reproduced this without cljs.test.

The minimal way to reproduce this needs two namespaces, where one depends on another. To display that the dependent namespace is not recompiled, it will print some compile time metadata from the another namespace.

src/hello/core.cljs

(ns hello.core
  (:require hello-world.test))

(enable-console-print!)

(println (:foo (meta #'hello.test/a)))

src2/hello/test.cljs

(ns hello.test)

(def ^{:foo :bar} a "foobar")

To build:

(require '[cljs.build.api :refer [build]])

(def opts {:main "hello.core"
           :output-to "out/main.js"
           :output-dir "out"
           :optimizations :none
           :verbose true})

(defn broken []
  (build "src" opts))

(defn working []
  " Note: ordering of the dirs matters
  (build (inputs "src2" "src" opts)))

To trigger the problem, the metadata needs to be changed after running the build once. Changing test ns doesn't cause core ns to recompiled and changing the metadata doesn't have any effect to println on core ns.

Activity

David Nolen made changes -
Field Original Value New Value
Fix Version/s Next [ 10651 ]
Affects Version/s 1.7.48 [ 10450 ]
David Nolen made changes -
Labels build
Juho Teperi made changes -
Assignee Juho Teperi [ deraen ]
Hide
Juho Teperi added a comment - - edited

First version for review.

Still missing support for finding sources for any build source parameter. Previously this was provided through Compilable interface but that doesn't make sense anymore as we don't want to compile them but find the namespaces.

Function naming could use some thinking.

This includes minimally changed alternative build function: build-new. I think future improvements to recompile-dependents would be limited to compile-sources where it would be possible to mark all dependent namespaces for recompile.

Show
Juho Teperi added a comment - - edited First version for review. Still missing support for finding sources for any build source parameter. Previously this was provided through Compilable interface but that doesn't make sense anymore as we don't want to compile them but find the namespaces. Function naming could use some thinking. This includes minimally changed alternative build function: build-new. I think future improvements to recompile-dependents would be limited to compile-sources where it would be possible to mark all dependent namespaces for recompile.
Juho Teperi made changes -
Attachment compile-dependency-order.patch [ 15041 ]
Hide
Juho Teperi added a comment -

Separate patches for deps bits and another for alternative build implementation.

Show
Juho Teperi added a comment - Separate patches for deps bits and another for alternative build implementation.
Juho Teperi made changes -
Attachment compile-dependency-order-a.patch [ 15059 ]
Attachment compile-dependency-order-b.patch [ 15060 ]
Hide
Juho Teperi added a comment -

A patch contains the new functions.
B modifies build to use new pipeline.

Notes:

To support different kind of source parameters, I added new method to Compilable protocol which only finds the source files in given source (dir, file, url...). There is still problem with supporting giving the source as list or vector, as I'm not sure if it's possible to create IJavaScript map from already read source.

Compile-file is still doing the recompile-dependents logic which is not optimal, but it's probably best to implement those changes separately.

Show
Juho Teperi added a comment - A patch contains the new functions. B modifies build to use new pipeline. Notes: To support different kind of source parameters, I added new method to Compilable protocol which only finds the source files in given source (dir, file, url...). There is still problem with supporting giving the source as list or vector, as I'm not sure if it's possible to create IJavaScript map from already read source. Compile-file is still doing the recompile-dependents logic which is not optimal, but it's probably best to implement those changes separately.
Juho Teperi made changes -
Attachment cljs-1437-2b.patch [ 15093 ]
Attachment cljs-1437-2a.patch [ 15094 ]
Hide
Juho Teperi added a comment -

I noticed I forgot to update build function to use new -find-sources method. Fixed in 3a and 3b patches.

Show
Juho Teperi added a comment - I noticed I forgot to update build function to use new -find-sources method. Fixed in 3a and 3b patches.
Juho Teperi made changes -
Attachment cljs-1437-3a.patch [ 15095 ]
Attachment cljs-1437-3b.patch [ 15096 ]
Hide
David Nolen added a comment -

Please remove all patches that don't just implement the new pipeline. Thanks! It just makes figuring out what patch to look at that much more complicated.

Show
David Nolen added a comment - Please remove all patches that don't just implement the new pipeline. Thanks! It just makes figuring out what patch to look at that much more complicated.
Juho Teperi made changes -
Attachment compile-dependency-order-a.patch [ 15059 ]
Juho Teperi made changes -
Attachment compile-dependency-order-b.patch [ 15060 ]
Juho Teperi made changes -
Attachment compile-dependency-order.patch [ 15041 ]
Juho Teperi made changes -
Attachment cljs-1437-2a.patch [ 15094 ]
Juho Teperi made changes -
Attachment cljs-1437-2b.patch [ 15093 ]
Hide
Juho Teperi added a comment -

Removed old patches.

Show
Juho Teperi added a comment - Removed old patches.
Juho Teperi made changes -
Attachment cljs-1437-3b.patch [ 15096 ]
Hide
Juho Teperi added a comment -

I noticed I had accidentally committed automatic version changes in second patch. Fixed now.

Show
Juho Teperi added a comment - I noticed I had accidentally committed automatic version changes in second patch. Fixed now.
Juho Teperi made changes -
Attachment cljs-1437-3b.patch [ 15106 ]
David Nolen made changes -
Assignee Juho Teperi [ deraen ] David Nolen [ dnolen ]
David Nolen made changes -
Status Open [ 1 ] In Progress [ 3 ]
David Nolen made changes -
Fix Version/s Backlog [ 10653 ]
Fix Version/s Next [ 10651 ]
Affects Version/s Next [ 10651 ]
Affects Version/s 1.7.48 [ 10450 ]
Hide
Juho Teperi added a comment - - edited

I noticed that build still contained some old code which caused it to compile most sources twice.

Updated B patch.

Show
Juho Teperi added a comment - - edited I noticed that build still contained some old code which caused it to compile most sources twice. Updated B patch.
Juho Teperi made changes -
Attachment cljs-1437-4b.patch [ 15230 ]
Hide
Juho Teperi added a comment -

Combined the patches.

Show
Juho Teperi added a comment - Combined the patches.
Juho Teperi made changes -
Attachment cljs-1437-5.patch [ 15231 ]
Juho Teperi made changes -
Attachment cljs-1437-3a.patch [ 15095 ]
Juho Teperi made changes -
Attachment cljs-1437-3b.patch [ 15106 ]
Juho Teperi made changes -
Attachment cljs-1437-4b.patch [ 15230 ]
Hide
David Nolen added a comment -

The patch needs to be rebased on master also please actually squash the patch into a single commit thanks.

Show
David Nolen added a comment - The patch needs to be rebased on master also please actually squash the patch into a single commit thanks.
Hide
Juho Teperi added a comment -

Rebased and squashed.

Also, I think I found solution for cljs.js' dependency on cljs.core$macros. (Fixes https://github.com/mneise/cljs-1437-test)

Show
Juho Teperi added a comment - Rebased and squashed. Also, I think I found solution for cljs.js' dependency on cljs.core$macros. (Fixes https://github.com/mneise/cljs-1437-test)
Juho Teperi made changes -
Attachment cljs-1437-6.patch [ 15242 ]
Hide
Juho Teperi added a comment -

Updated patch to fix problem with recompile dependents. Fixed by adding cljs.compiler/inputs binding to cljs.closure/compiler-sources. This is needed so that cljs.compiler/compile-file can see the namespace data from current run and use that to check the ns-dependents.

Show
Juho Teperi added a comment - Updated patch to fix problem with recompile dependents. Fixed by adding cljs.compiler/inputs binding to cljs.closure/compiler-sources. This is needed so that cljs.compiler/compile-file can see the namespace data from current run and use that to check the ns-dependents.
Juho Teperi made changes -
Attachment cljs-1437-7.patch [ 15246 ]
Hide
Juho Teperi added a comment -

Updated the patch with a change to cljs.analyzer/locate-src. This fixes problem in a case where a file being compiled depends on a file which is not available on classpath and is not yet analyzed.

Show
Juho Teperi added a comment - Updated the patch with a change to cljs.analyzer/locate-src. This fixes problem in a case where a file being compiled depends on a file which is not available on classpath and is not yet analyzed.
Juho Teperi made changes -
Attachment cljs-1437-8.patch [ 15249 ]
Juho Teperi made changes -
Attachment cljs-1437-5.patch [ 15231 ]
Juho Teperi made changes -
Attachment cljs-1437-6.patch [ 15242 ]
Juho Teperi made changes -
Attachment cljs-1437-7.patch [ 15246 ]
David Nolen made changes -
Status In Progress [ 3 ] Resolved [ 5 ]
Resolution Completed [ 1 ]
David Nolen made changes -
Status Resolved [ 5 ] Closed [ 6 ]

People

Vote (1)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved: