<< Back to previous view

[CLJS-1437] Recompile dependents broken when files are not inside one directory Created: 30/Aug/15  Updated: 03/Nov/15  Resolved: 03/Nov/15

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: 1.7.145
Fix Version/s: 1.7.228

Type: Defect Priority: Major
Reporter: Juho Teperi Assignee: David Nolen
Resolution: Completed Votes: 1
Labels: build

Attachments: Text File cljs-1437-8.patch    

 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.



 Comments   
Comment by Juho Teperi [ 02/Sep/15 5:55 PM ]

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.

Comment by Juho Teperi [ 09/Sep/15 2:02 PM ]

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

Comment by Juho Teperi [ 22/Sep/15 1:05 PM ]

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.

Comment by Juho Teperi [ 22/Sep/15 1:14 PM ]

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

Comment by David Nolen [ 22/Sep/15 10:23 PM ]

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.

Comment by Juho Teperi [ 23/Sep/15 12:38 PM ]

Removed old patches.

Comment by Juho Teperi [ 29/Sep/15 2:57 PM ]

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

Comment by Juho Teperi [ 19/Oct/15 2:30 PM ]

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

Updated B patch.

Comment by Juho Teperi [ 19/Oct/15 2:43 PM ]

Combined the patches.

Comment by David Nolen [ 26/Oct/15 4:57 PM ]

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

Comment by Juho Teperi [ 27/Oct/15 12:39 PM ]

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)

Comment by Juho Teperi [ 29/Oct/15 2:38 PM ]

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.

Comment by Juho Teperi [ 01/Nov/15 2:33 AM ]

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.

Comment by David Nolen [ 03/Nov/15 6:04 PM ]

Fixed https://github.com/clojure/clojurescript/commit/409d1eca4fcf776be1f7b28f759d6f36f7a83ec8

Generated at Sat Apr 20 01:23:22 CDT 2019 using JIRA 4.4#649-r158309.