Open mfikes opened 8 years ago
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.
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]))
@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:
https://github.com/mfikes/specter/commit/eb31f0c96fb84f68f7be66c02f37f56605948373
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.
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.
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.
Thanks for the thorough explanation.
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.
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).
A patch to make cljs.test
usable from bootstrap has now landed. https://github.com/clojure/clojurescript/commit/28e040d41f3a83901b6391898f51c4bfc2622452
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. :)
Progress:
cljs.test
revisions for bootstrap have now been formally released with ClojureScript 1.8.51test.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-105The 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
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]
I tried modifying Specter master to see how far I could get with Planck, which the experimental changes here https://github.com/mfikes/specter/commit/bdef52461970f8a8bf77b46065e0858370247cd7
name-with-attributes
into the namespace to avoid the dep.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.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.
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.
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.)
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:
https://github.com/mfikes/specter/commit/084be5cffed623aefef9a05157bb3ae8c5506a54
(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}]
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?
Yeah, my rough thoughts on tests:
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.)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: https://github.com/mfikes/andare/commit/8bffb637081d6d1476d042be36fe75074e1a44c7 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.
Hey Nathan, I tried the latest (after your changes for bootstrap). One require/alias is needed: https://github.com/mfikes/specter/commit/7b6e32ff590c8f03a5a096fb63ff0a516ea71c9a
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).
I tried adding that require but now I get a "No such namespace: cljs.js" error when running the tests in the node repl.
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: https://github.com/mfikes/specter/commit/6df3fa016de2d89dc5d86bc18951533a7d89be1d
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.
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 :)
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)?
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?
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 inprofile.clj
—like it has been so far.
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.
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.
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).
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.
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.
@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%0A%0A%0A(s%2Ftransform%20%5Bs%2FMAP-VALS%20s%2FMAP-VALS%5D%0A%20%20%20%20%20%20%20%20%20%20%20%20%20%20inc%0A%20%20%20%20%20%20%20%20%20%20%20%20%20%20%7B%3Aa%20%7B%3Aaa%201%7D%20%3Ab%20%7B%3Aba%20-1%20%3Abb%202%7D%7D)%0A%0A&external-libs=%5Bhttps%3A%2F%2Fraw.githubusercontent.com%2Fnathanmarz%2Fspecter%2Fmaster%2Fsrc%2Fclj%2F%5D).
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}}}
@viebel No update since September. First step is to get the tests running properly so that we can ensure Bootstrap support moving forward.
@mfikes The codebase is updated to test.check 0.9.0, so I think we're ready now for a bootstrap test script.
Cool. I'll see if I can put one together this weekend.
@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:
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
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!
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.
@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?
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
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.
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.
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.
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.
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))))
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.
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#L2It's tedious, but if this is instead revised to a
:require
with explicit:refer
s, 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-L21With 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:
(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 viabrew 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.