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

Support bootstrap ClojureScript #72

Open
mfikes opened this issue Apr 16, 2016 · 47 comments
Open

Support bootstrap ClojureScript #72

mfikes opened this issue Apr 16, 2016 · 47 comments

Comments

@mfikes
Copy link

mfikes commented Apr 16, 2016

With some mild changes, bootstrap ClojureScript can be supported by Specter, thus broadening its usability with that target environment.

The main thing that would need to be revised is allowing the macros namespace to be compiled as ClojureScript code, and the only thing that currently prevents this is the use of :use here https://github.com/nathanmarz/specter/blob/585637b4fe3529086f05ad09d07266da42463c6e/src/clj/com/rpl/specter/macros.clj#L2

It's tedious, but if this is instead revised to a :require with explicit :refers, then things work. Here's an experimental change I made in a fork: https://github.com/mfikes/specter/blob/48c6f7b4876e37c734489e1882c17dc9a293459e/src/clj/com/rpl/specter/macros.clj#L2-L21

With this, I am able to use Specter with Planck, and I suspect that Specter would be generally usable in any bootstrapped ClojureScript environment.

Here is an example:

$ planck -c target/specter-0.9.3.jar
Planck 1.11
ClojureScript 1.8.44
    Docs: (doc function-name-here)
          (find-doc "part-of-name-here")
  Source: (source function-name-here)
    Exit: Control+D or :cljs/quit or exit or quit
 Results: Stored in vars *1, *2, *3, an exception in *e

cljs.user=> (require '[com.rpl.specter :refer [transform ALL]])
nil
cljs.user=> (transform [ALL :a even?]
       #_=>               inc
       #_=>               [{:a 1} {:a 2} {:a 4} {:a 3}])
[{:a 1} {:a 3} {:a 5} {:a 3}]

(Note that, to do the above requires master of Planck, because the currently released version of Planck doesn't have support for the clojure.core.reducers namespace. Master of Planck is installable via brew install --HEAD planck.)

I also tried running Specter's unit tests using Planck. This is evidently currently not yet possible because of something that needs to be sorted with clojure.test.check for bootstrap ClojureScript, but I suspect that this issue is resolvable, and with that, it would be possible to ensure Specter formally passes its test for this environment.

@nathanmarz
Copy link
Collaborator

I'm 100% on board with this. I changed the code on master to alias the namespace for impl instead of doing a use. Is this now compatible with Planck? Also, very soon Specter will no longer be using clojure.core.reducers for the cljs version.

@mfikes
Copy link
Author

mfikes commented Apr 18, 2016

Thanks @nathanmarz !

I took a quick look at your latest commit. I haven't tried it yet, but it looks like you are using prefix list notation in the ns form, which isn't supported in ClojureScript.

This should work though:

(ns com.rpl.specter.macros
  (:require [com.rpl.specter]
                 [com.rpl.specter.impl :as i]))

@mfikes
Copy link
Author

mfikes commented Apr 18, 2016

@nathanmarz Following up on my last comment:

Yes, just to confirm, I tried Specter master and the prefix list notation causes an issue for ClojureScript:

cljs.user=> (require '[com.rpl.specter :refer [transform ALL]])
Could not eval com.rpl.specter.macros
Only :as alias and :refer (names) options supported in :require; offending spec: [com.rpl.specter [impl :as i]] at line 1 
nil

FWIW, the prefix-list ClojureScript constraint is covered here: https://github.com/clojure/clojurescript/wiki/Differences-from-Clojure#namespaces

More importantly, though, my comment above has an issue in that circularity exists. The [com.rpl.specter] spec can't be there otherwise the ns processing will go into a loop:

cljs.user=> (require '[com.rpl.specter :refer [transform ALL]])
Could not require com.rpl.specter
Maximum call stack size exceeded.

Removing it, causes things to work. Here is the exact change I tried, after updating to your upstream changes and then fixing things to not use prefix-list notation:

mfikes@eb31f0c

@nathanmarz
Copy link
Collaborator

OK, I made the change in master. What's the easiest way to get the tests to run against bootstrap cljs? I'd like to make sure that continues to be supported as the project evolves.

@mfikes
Copy link
Author

mfikes commented Apr 18, 2016

Currently Planck is the only environment I'm aware of that supports cljs.test in bootstrap mode (I had to make some minor changes to port that namespace.) I'm hoping we can revise the version of cljs.test that ships with the compiler, thus making it easier for projects like Specter to run their tests in bootstrap mode.

I also plan on sorting what is going on with test.check—this should lead to an ability to at least run Specter's unit tests with Planck in the interim.

This is not ideal—even the ClojureScript compiler currently can't run its own unit tests in bootstrap mode, while Planck can run the ClojureScript compiler's tests. So the goal would be to push this capability upstream so Planck need not be the only way to address testing.

@mfikes
Copy link
Author

mfikes commented Apr 18, 2016

FYI, David Nolen expressed interest / approval in a patch that would make cljs.test usable for bootstrap environments. See http://dev.clojure.org/jira/browse/CLJS-1626 This would be a big step towards addressing Nathan's last comment.

@nathanmarz
Copy link
Collaborator

Thanks for the thorough explanation.

@nathanmarz
Copy link
Collaborator

OK, the master branch no longer uses core.reducers for the cljs version. Now all that's left for this is sorting out the test stuff.

@mfikes
Copy link
Author

mfikes commented Apr 18, 2016

Cool. I confirmed that the released version of Planck (1.10) works with Specter master.

(As an aside, I had to tell Planck to enable :static-fns true by additionally passing -s to Planck. This has nothing really to do with Specter, but is a defect in JavaScriptCore, covered here http://dev.clojure.org/jira/browse/CLJS-910).

@mfikes
Copy link
Author

mfikes commented Apr 21, 2016

A patch to make cljs.test usable from bootstrap has now landed. clojure/clojurescript@28e040d

Also, a repo exists showing (at lest one way) how to use this downstream: https://github.com/mfikes/titrate

So, things remaining to get support for testing Specter are the ClojureScript support to be formally released, and sorting out things like test.check in bootstrap. :)

@mfikes
Copy link
Author

mfikes commented May 1, 2016

Progress:

  1. The cljs.test revisions for bootstrap have now been formally released with ClojureScript 1.8.51
  2. test.check can be made to work with bootstrap, with work proceeding with maintainer of that lib "in the loop" via http://dev.clojure.org/jira/browse/TCHECK-105

@nathanmarz
Copy link
Collaborator

The code in master may have some breaking changes with regards to bootstrap support. In particular: https://github.com/nathanmarz/specter/blob/master/src/clj/com/rpl/specter/macros.clj#L369

  • Will the hack to determine in the macro whether emitting for cljs or clj work in bootstrap?
  • What should be used to retrieve a uuid string when run in cljs rather than java.util.UUID?

@mfikes
Copy link
Author

mfikes commented May 23, 2016

Hi Nathan, yes, the hack works. In particular for this source:

(ns foo.core)

(defmacro testme []
  (if (contains? &env :locals) :cljs :clj))

at the Planck REPL:

cljs.user=> (require-macros 'foo.core)
nil
cljs.user=> (foo.core/testme)
:cljs

For UUIDs, while ClojureScript has a UUID type (supporting the tagged literal), it appears that people resort to random number generation. (I looked and didn't see anything in the Google Closure Library).

Since you are worried about birthday collisions, this SO http://stackoverflow.com/a/2117523 has some analysis that seems to imply a 1 in a million chance.

Easy enough to do in ClojureScript using the (private) js* special form:

cljs.user=> (js* "'xxxxxxxx-xxxx-4xxx-yxxx-xxxxxxxxxxxx'.replace(/[xy]/g, function(c) {var r = Math.random()*16|0,v=c=='x'?r:r&0x3|0x8;return v.toString(16);});")
"d371220d-229b-4259-b3e0-92f3559d06fb"

And, (it doesn't appear you need it), but there is a constructor in cljs.core:

cljs.user=> (uuid "d371220d-229b-4259-b3e0-92f3559d06fb")
#uuid "d371220d-229b-4259-b3e0-92f3559d06fb"

I tried Specter master in Planck and it hits the fact that this namespace is not available: [clojure.tools.macro :as m]

@mfikes
Copy link
Author

mfikes commented May 23, 2016

I tried modifying Specter master to see how far I could get with Planck, which the experimental changes here mfikes@bdef524

  • I copied name-with-attributes into the namespace to avoid the dep.
  • Since walk/macroexpand-all doesn't exist in ClojureScript, I cobbled it together based on an idea in this post. Since this is just an experimental hack, this pulls in cljs.js as a dep, which makes the namespace specific to ClojureScript in the fork / hack.
  • I hacked the lack of UUID issue by just generating a random string of 50 digits

With this I could at least load things into Planck, but not good with runtime behavior yet:

cljs.user=> (require-macros '[com.rpl.specter.macros :refer [transform]])
nil
cljs.user=> (require '[com.rpl.specter :refer [ALL]])
nil
cljs.user=> (transform [ALL :a even?]
       #_=>               inc
       #_=>               [{:a 1} {:a 2} {:a 4} {:a 3}])
undefined is not an object (evaluating 'clojure.lang.Cons') at line 1

I suppose the value in the above is not so much in helping identity fixes, but at least in pointing out the things that break with self-host.

@nathanmarz
Copy link
Collaborator

OK, good to know. I also pushed some new changes (bug fixes) that required me to use riddley.walk instead of clojure.walk, but that only affects the Clojure implementation. So using your walk/macroexpand-all replacement should be fine for ClojureScript.

I did a little googling on proper UUID's in javascript, and the results were not encouraging. The best approach might be to remove the reliance on UUID's for the ClojureScript version. David Nolen had an idea about doing dynamic calls to def and exists? for the inline caches, something that can't be done in Clojure.

Not sure what that evaluation error is all about – if you change the code to do an (i/spy ...) around the output of the com.rpl.specter.macros/path macro that could give some insight.

@mfikes
Copy link
Author

mfikes commented May 25, 2016

I'll see what I can figure out using i/spy.

Another thing to consider is changing the macros.clj file to be conditional (cljx or cljc) so that the macroexpand aspect can be employed only when using self-hosted ClojureScript. (In regular ClojureScript there is no issue given the macros are compiled as Clojure.)

@mfikes
Copy link
Author

mfikes commented May 28, 2016

The clojure.lang.Cons issue was fairly straightforward (and quick to pinpoint with a quick (pst) after it occurs): It is some Clojure-specific code in fn-invocation? that can be worked around:

mfikes@084be5c

(Note that, with respect to the change above, (list? (cons 1 ())) yields true in ClojureScript.)

With this, things work, at least for this sequence :)

cljs.user=> (require-macros '[com.rpl.specter.macros :refer [transform]])
nil
cljs.user=> (require '[com.rpl.specter :refer [ALL]])
nil
cljs.user=> (transform [ALL :a even?] inc [{:a 1} {:a 2} {:a 4} {:a 3}])
[{:a 1} {:a 3} {:a 5} {:a 3}]

@nathanmarz
Copy link
Collaborator

OK, I made all these changes in Specter and the tests are still passing. Can you verify the tests pass under bootstrap?

The last remaining action item is handling the UUID issue properly. I'll look into this more – at the moment it's using your random string code.

There's also room for further improvement later on to improve the path macro when run in bootstrap environment vs. normal cljs environment. Since bootstrap has eval, the same strategy as used by the clj implementation can be used for inline caching. The end effect of this is improved performance. I'll open an issue for this.

Finally, to ensure bootstrap support is maintained for future releases, there needs to be an easy way to run the tests via bootstrap – like lein bootstrap test or something like that. What's the best way to do this?

@mfikes
Copy link
Author

mfikes commented May 29, 2016

Yeah, my rough thoughts on tests:

  1. A build of test.check needs to be available that supports bootstrap. (Either the patch in http://dev.clojure.org/jira/browse/TCHECK-105 needs to be accepted or a custom build needs to be made using that patch.)
  2. Then scripts and a little bit of bootstrap / self-host code can be added to the Specter tree making it possible to run the Specter tests in a bootstrap environment.

The idea behind (2) is it causes the Specter code to actually be loaded and compiled by the bootstrapped ClojureScript compiler. This approach is being used, for example in a port I have of core.async that targets bootstrapped ClojureScript: mfikes/andare@8bffb63 This same technique is used in the ClojureScript compiler itself to ensure its own tests work in bootstrap mode, and also in the test.check patch. A repo showing targeting all three of Clojure / ClojureScript JVM and bootstrap ClojureScript is at https://github.com/mfikes/titrate

The unfortunate aspect of this is that it requires you to (a) get the cljs.jar (since Specter itself targets an older version of ClojureScript), and (b) have Node.js. But once those annoying bits are addressed, it really boils down to another test script to run.

I'm happy to help put this stuff together. (Perhaps in a separate branch you could copy in, or as a PR, or whatever mechanism works best for you.)

I kind-of wish the test.check patch would go through... that's the main bit I think this is waiting on.

Then, of course, there might be issues with Specter itself, getting it to run through tests in this way—but that's I suppose what you want: To identify anything that might be broken in some corner of the code.

@mfikes
Copy link
Author

mfikes commented May 29, 2016

Hey Nathan, I tried the latest (after your changes for bootstrap). One require/alias is needed: mfikes@7b6e32f

@mfikes
Copy link
Author

mfikes commented May 29, 2016

Oh Nathan, another thing I forgot to mention regarding test.check: It's API (or at least its namespaces) has been revised with the latest releases (that's where the self-host patch is pending).

@nathanmarz
Copy link
Collaborator

I tried adding that require but now I get a "No such namespace: cljs.js" error when running the tests in the node repl.

@mfikes
Copy link
Author

mfikes commented May 29, 2016

Ahh. OK. cljx must have different semantics than reader conditionals. (FWIW reader conditionals will take the :cljs branch in macro namespaces when being compiled under bootstrap, but the :clj branch when macro namespaces are being compiled for regular JVM-based ClojureScript.)

Given that, this works in bootstrap: mfikes@6df3fa0

It relies on the fact that, by definition, if you are in bootstrap, then cljs.js has already been loaded. So, it is abusing the concept of using a namespace that hasn't been explicitly required, but banking on the implicit aspect.

@nathanmarz
Copy link
Collaborator

Made that change and now the tests run fine.

As for getting the tests running under bootstrap, a pull request with the requisite changes would be great (once all the test.check stuff you mentioned is sorted out). Will leave this issue open until those changes are made as that is the time when bootstrap compatibility can be easily enforced moving forward. Until then the bootstrap test runner will be an remote procedure call to @mfikes :)

@mfikes
Copy link
Author

mfikes commented May 29, 2016

Cool. Do you think Specter can be updated to the latest test.check release and API? Or, does that cause an issue with Specter striving to be backwards compatible (and usable in older ClojureScript environments)?

@nathanmarz
Copy link
Collaborator

nathanmarz commented May 29, 2016

I actually don't know what versions of ClojureScript are important to keep supporting. So far I've been trying to support as many versions of Clojure and ClojureScript as possible – but now that 1.9.0 is on its way I'm open to dropping compatibility guarantees for older versions. Based on reading the test.check changelog it looks like I would have to upgrade to Clojure 1.7.0 (and I'm assuming ClojureScript 1.7.x as well?) Since you know a lot more about ClojureScript than I do, what are your thoughts on which ClojureScript versions are important to maintain compatibility with?

@mfikes
Copy link
Author

mfikes commented May 29, 2016

Back around the time this post occurred http://swannodette.github.io/2014/12/31/the-old-way-the-new-way/ there were some breaking changes that causes a bit of difficulty with forwards and backwards compatibility. Since that time frame, it has essentially been a continuous (roughly monthly) sequence of releases without any real compatibility hiccups or important milestones that often come up as a compatibility discontinuity.

So, I'd suggest supporting back as far as possible. But to be honest, I don't know how to determine what that might be. For example, if you require Clojure 1.7.0, I think versions of ClojureScript prior to 1.7.28 still work.

In terms of supporting self-host, consumers of Specter will need 1.7.28 or later (http://swannodette.github.io/2015/07/29/clojurescript-17/) but to be honest, such users are likely to be on a much more recent version because a lot of fixes have occurred in self-host functionality. But, you can almost set up a specific self host profile or copy in a released cljs.jar, so I think the self-host aspect can be a bit decoupled from what you choose to put in profile.clj—like it has been so far.

@mfikes
Copy link
Author

mfikes commented May 29, 2016

Speaking of UUIDs and test.check, this is interesting relevant prior art: https://github.com/clojure/test.check/blob/a9e15ab4d884097f2e0636e98d047f46a20b7cc8/src/main/clojure/clojure/test/check/generators.cljc#L1276-L1309

Edit: I suppose in test.check's use case, they need to honor a seed to generate the same sequence of UUIDs.

@nathanmarz
Copy link
Collaborator

Thanks for the info. I think I'll release 0.11.0 targeting the versions it always has, and for 0.12.0 I'll upgrade to Clojure 1.7.0 and the lowest version of ClojureScript that will enable everything we need with bootstrap.

Also, I removed the reliance on UUID's for the ClojureScript implementation so we don't have to worry about that anymore.

@mfikes
Copy link
Author

mfikes commented Sep 7, 2016

Support for bootstrapped ClojureScript has landed in the test.check repo via TCHECK-105.

In preparation for when the JAR is available, I'm happy to help set up a script that can run Specter's unit tests completely in self-hosted mode (either under Node or Nashorn if you have a preference).

It also looks like Specter needs to be updated from test.check 0.7.0 to 0.9.x (which appears to involve some breaking changes in the the test.check API (I don't have a feel for what the work entails here, other than I know if you try to change the dep to the latest released test.check in Specter's project.clj file, things don't work).

@nathanmarz
Copy link
Collaborator

That test script would be greatly appreciated. I'd prefer Node just because that's what I already have installed.

I'll take a look at updating test.check to 0.9.x.

@mfikes
Copy link
Author

mfikes commented Sep 7, 2016

Cool, when #144 is ready, I'll try it with a script (something like script/test-self-host) that runs the test suite completely within Node, using a locally-built copy of test.check master.

@viebel
Copy link

viebel commented Jan 14, 2017

@mfikes @nathanmarz what's the current status of specter support for bootstrap clojurecript?
Are you guys able to run specter in planck?
I was not able to run specter master on klipse; here is a live demo.

I've tried this code:

(ns my.ns
  (:require [com.rpl.specter :as s]))


(s/transform [s/MAP-VALS s/MAP-VALS]
              inc
              {:a {:aa 1} :b {:ba -1 :bb 2}})

And I got this error

#error {:message "Could not parse ns form com.rpl.specter.impl", :data {:tag :cljs/analysis-error}
:cause #error {:message "Invalid :refer, macro com.rpl.specter.util-macros/doseqres does not exist"
:data {:tag :cljs/analysis-error}}}

@nathanmarz
Copy link
Collaborator

@viebel No update since September. First step is to get the tests running properly so that we can ensure Bootstrap support moving forward.

@nathanmarz
Copy link
Collaborator

@mfikes The codebase is updated to test.check 0.9.0, so I think we're ready now for a bootstrap test script.

@mfikes
Copy link
Author

mfikes commented Mar 10, 2017

Cool. I'll see if I can put one together this weekend.

mfikes added a commit to mfikes/specter that referenced this issue Mar 11, 2017
@mfikes
Copy link
Author

mfikes commented Mar 12, 2017

@nathanmarz I put together a Leiningen plugin that essentially leverages config from project.clj and then simply shells out to installed copies of either Lumo or Planck, feeding them the necessary arguments, and running the project's unit tests within them. (Delegating to Lumo or Planck is simpler than synthesizing a self-host environment to run in Node.js.)

The plugin is Tach and is deployed to Clojars.

I've dog-fooded it on Andare (my self-host port of core.async), and it was as simple as just adding the plugin and then lein tach lumo or lein tach planck.

For Specter, I tried it with 4 changes:

  • Add the plugin
  • Put it in debug mode so we can see what is going on
  • Specify a new test runner ns
  • Revise Specter's project.clj to depend on [org.clojure/test.check "0.9.1-SNAPSHOT"]

Here is the git diff:

   :auto-clean false
   :dependencies [[riddley "0.1.12"]]
   :plugins [[lein-codox "0.9.5"]
-            [lein-doo "0.1.7"]]
+            [lein-doo "0.1.7"]
+            [lein-tach "0.1.0"]]
+
   :codox {:source-paths ["target/classes" "src/clj"]
           :namespaces [com.rpl.specter
                        com.rpl.specter.zipper
@@ -27,8 +29,11 @@
                                    :main 'com.rpl.specter.cljs-test-runner
                                    :optimizations :simple}}]}

+  :tach {:test-runner-ns 'com.rpl.specter.cljs-self-test-runner
+         :debug? true}
+
   :profiles {:dev {:dependencies
-                   [[org.clojure/test.check "0.9.0"]
+                   [[org.clojure/test.check "0.9.1-SNAPSHOT"]
                     [org.clojure/clojure "1.8.0"]
                     [org.clojure/clojurescript "1.9.229"]]}

With test/com/rpl/specter/cljs_self_test_runner.cljs containing:

(ns com.rpl.specter.cljs-self-test-runner
  (:require [cljs.test :refer-macros [run-tests]]
            [com.rpl.specter.core-test]
            [com.rpl.specter.zipper-test]))

(run-tests 'com.rpl.specter.core-test
           'com.rpl.specter.zipper-test)

With this, running lein tach planck or lein tach lumo, things go into an infinite loop in the require sequence, which can be seen if you take the command line that is dumped by Tach, and then revise run it manually with either Planck or Lumo but by adding a -v to see more of what is going on. Both end up logging a repeating sequence that looks like this:

Evaluating com.rpl.specter
Namespace side effects for com.rpl.specter$macros
Loading dependencies for com.rpl.specter$macros
Loading com.rpl.specter.protocols namespace
Loading result:  {:value true}
Loading dependencies for com.rpl.specter$macros
Loading com.rpl.specter.impl namespace
Loading result:  {:value true}
Loading dependencies for com.rpl.specter$macros
Loading com.rpl.specter.navs namespace
Evaluating com.rpl.specter.navs
Namespace side effects for com.rpl.specter.navs
Loading dependencies for com.rpl.specter.navs
Loading com.rpl.specter.impl namespace
Loading result:  {:value true}
Loading dependencies for com.rpl.specter.navs
Loading clojure.walk namespace
Loading result:  {:value true}
Loading dependencies for com.rpl.specter.navs
Processing :use-macros for com.rpl.specter.navs
Loading com.rpl.specter.util-macros macros namespace
Loading com.rpl.specter macros namespace
Evaluating com.rpl.specter
Namespace side effects for com.rpl.specter$macros

@cap10morgan
Copy link

What's the latest on this? I just tried to use specter in a planck-run CLJS CLI tool I'm working on and was sad to find that it wasn't yet supported. But it sounds like the blockers are unblocked based on skimming the above. I'd be happy to help move this forward if there's anything helpful I can do. Thanks!

@nathanmarz
Copy link
Collaborator

This is pretty low on my priority use since I don't use bootstrap at all. However, the issue is definitely unblocked at this point, and I'd welcome a contribution to fix this.

@cap10morgan
Copy link

@nathanmarz Cool, thanks for the follow up. It’s not totally clear to me what sort of contribution is needed at this point. But maybe @mfikes knows?

@nathanmarz
Copy link
Collaborator

Need to get the tests running in Planck and then fix all the resulting errors. The changes needed to Specter to support bootstrap should all be within the inline caching macro: https://github.com/nathanmarz/specter/blob/master/src/clj/com/rpl/specter.cljc#L229

@jeff303
Copy link
Contributor

jeff303 commented Sep 28, 2020

Permalink for the last comment (it's actually just in defmacro path). https://github.com/redplanetlabs/specter/blob/1.0.4/src/clj/com/rpl/specter.cljc#L229

I've rebased this again onto master and tried to incorporate all of the stuff @mfikes had mentioned here: https://github.com/jeff303/specter/tree/bootstrap

When I try lein tach planck now (the verbose version, that is) is, I'm seeing:

Pre-file side-effects /Users/jeff/dev/redplanetlabs/specter/src/clj/com/rpl/specter.cljc
Evaluating com.rpl.specter
Namespace side effects for com.rpl.specter$macros
Loading dependencies for com.rpl.specter$macros
Loading com.rpl.specter.protocols namespace
Loading result: {:value true}
Loading dependencies for com.rpl.specter$macros
Loading com.rpl.specter.impl namespace
Loading result: {:value true}
Loading dependencies for com.rpl.specter$macros
Loading com.rpl.specter.navs namespace
Pre-file side-effects /Users/jeff/dev/redplanetlabs/specter/src/clj/com/rpl/specter/navs.cljc
Evaluating com.rpl.specter.navs
Namespace side effects for com.rpl.specter.navs
Loading dependencies for com.rpl.specter.navs
Loading com.rpl.specter.impl namespace
Loading result: {:value true}
Loading dependencies for com.rpl.specter.navs
Processing :use-macros for com.rpl.specter.navs
Loading com.rpl.specter.util-macros macros namespace
Loading com.rpl.specter macros namespace
Pre-file side-effects /Users/jeff/dev/redplanetlabs/specter/src/clj/com/rpl/specter.cljc
<above sequence repeats ad infinitum>

To me this looks like a fairly straightforward circular dependency thing. I.e. navs namespace is depending on specter namespaces, which depends on navs, and so on. Will keep messing with it when I get some time.

jeff303 pushed a commit to jeff303/specter that referenced this issue Sep 28, 2020
@jeff303
Copy link
Contributor

jeff303 commented Sep 28, 2020

Made some more progress on this, I think: https://github.com/jeff303/specter/tree/bootstrap

Solved the circular dependency problem, and bringing in the cgrand/macrovich plugin seemed to go a long way. Now, the problems seem to be with:

  • *compile-files*
  • extend

which, based on my research, is somewhat expected since those aren't available in CLJS.

@jeff303
Copy link
Contributor

jeff303 commented Sep 29, 2020

Spent a bit more time trying to create a new version of extend-protocolpath that uses extend-protocol instead of extend (since the latter isn't available in cljs). I'm having trouble because it doesn't seem possible to specify the protocol impl method in exactly the same way (extend takes a map, and the keys for the protocol method names can be fn literals, whereas extend-protocolpath is expecting params, then body). So tests still aren't passing.

@jeff303
Copy link
Contributor

jeff303 commented Oct 1, 2020

OK, finally making some real progress. With the latest changes (more commits on same branch, linked above), the tests actually run (and mostly pass) in self-hosted mode.

lein tach planck
...
Ran 140 tests containing 472 assertions.
12 failures, 0 errors.

What's still failing is inline-caching-test, which is unsurprising. Will work more on that when I can.

@jeff303
Copy link
Contributor

jeff303 commented Oct 9, 2020

Some further testing has revealed that this is the only case failing in the bootstrap run (responsible for 3*2*2=12 assertion failures).

  (ic-test
    [v]
    [s/ALL (double-str-keypath v (inc v))]
    str
    [{"12" :a "1011" :b} {"1011" :c}]
    [[1] [10]])

double-str-keypath was defined above:

(defn ^:direct-nav double-str-keypath [s1 s2]
  (path (s/keypath (str s1 s2))))

@jeff303
Copy link
Contributor

jeff303 commented Oct 14, 2020

Still more debugging (this time in a planck REPL) reveals something interesting:

cljs.user=> (s/select [s/ALL (double-str-keypath 1 (inc 1))] [{"12" :a "1011" :b} {"1011" :c}])
[nil nil]
cljs.user=> (s/select [s/ALL (double-str-keypath 1 2)] [{"12" :a "1011" :b} {"1011" :c}])
[:a nil]

So, basically, the (inc 1) arg isn't being evaluated properly, or at the right time for navigation. Maybe this is because cljs defines both a macro and a function for cljs.core/inc?

In any case, if I change the test to use (+ 1 v) instead of (inc v), then everything passes! See updated branch.

Edit: actually, just referring to the correct symbol depending on the environment seems to work. Updated branch again.

jeff303 added a commit to jeff303/specter that referenced this issue Oct 22, 2020
Adding self-host testing infrastructure and scripts (including tach
plugin)

Incorporating cgrand/macrovich plugin to separate macro definitions from usage in specter.cljc

Changing tests to use :require with :refer-macros to bring in cljs.test macros in cljs mode

Rewriting extend-protocolpath to use extend-protocol, which is available in cljs (extend is not)

Fixing ic-prepare-path implementation to use VarUse even in hosted mode (hardcoding the condition to false)

Various changes in test files to get them working in all three modes

In test, using correct symbol for inc depending on the environment
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

No branches or pull requests

5 participants