-
Notifications
You must be signed in to change notification settings - Fork 105
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
Comments
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 This should work though:
|
@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:
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
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: |
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 I also plan on sorting what is going on with 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 |
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 |
A patch to make 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 |
Progress:
|
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
|
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:
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)
And, (it doesn't appear you need it), but there is a constructor in
I tried Specter master in Planck and it hits the fact that this namespace is not available: |
I tried modifying Specter master to see how far I could get with Planck, which the experimental changes here mfikes@bdef524
With this I could at least load things into Planck, but not good with runtime behavior yet:
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 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 Not sure what that evaluation error is all about – if you change the code to do an |
I'll see what I can figure out using Another thing to consider is changing the |
The (Note that, with respect to the change above, With this, things work, at least for this sequence :)
|
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 Finally, to ensure bootstrap support is maintained for future releases, there needs to be an easy way to run the tests via bootstrap – like |
Yeah, my rough thoughts on tests:
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 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 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: mfikes@7b6e32f |
Oh Nathan, another thing I forgot to mention regarding |
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. Given that, this works in bootstrap: mfikes@6df3fa0 It relies on the fact that, by definition, if you are in bootstrap, then |
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 |
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.
|
Speaking of UUIDs and Edit: I suppose in |
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 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 |
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 |
@mfikes @nathanmarz what's the current status of specter support for bootstrap clojurecript? 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 The plugin is Tach and is deployed to Clojars. I've dog-fooded it on Andare (my self-host port of For Specter, I tried it with 4 changes:
Here is the git diff:
With
With this, running
|
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 I've rebased this again onto When I try
To me this looks like a fairly straightforward circular dependency thing. I.e. |
Made some more progress on this, I think: https://github.com/jeff303/specter/tree/bootstrap Solved the circular dependency problem, and bringing in the
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 |
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.
What's still failing is |
Some further testing has revealed that this is the only case failing in the bootstrap run (responsible for
|
Still more debugging (this time in a
So, basically, the In any case, if I change the test to use Edit: actually, just referring to the correct symbol depending on the environment seems to work. Updated branch again. |
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
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.The text was updated successfully, but these errors were encountered: