Fork me on GitHub
#cljs-dev
<
2017-07-09
>
mfikes02:07:55

In the context of cljs-oss had success setting up Planck and triggering it to build with a specific version of ClojureScript. Here is a sketch: 1. I changed Planck's project.clj to have a dep that can be driven by an environment variable: [org.clojure/clojurescript ~(or (System/getenv "CLOJURESCRIPT_VERSION") "1.9.660")] 2. I added a minimal "`.travis.yml`-only" repo at https://github.com/cljs-oss/planck which clones and builds Planck and runs its tests 3. For the sake of running it in Travis, I forked this repo to my own GitHub account, but this step is just an optional pragmatic bit for now 4. I installed the travis client, logged in using a GitHub personal access token created for this purpose, and then had it give me a Travis API token 5. I chose which ClojureScript version I wanted to trigger the Planck build with: body='{"request": {"branch":"master", "config": {"env": {"CLOJURESCRIPT_VERSION": "1.9.671"}}}}' 6. I triggered the build: curl -s -X POST -H "Content-Type: application/json" -H "Accept: application/json" -H "Travis-API-Version: 3" -H "Authorization: token XXtokenhereXXXX" -d "$body" 7. Observed the environment variable being set in the Travis web UI log: export CLOJURESCRIPT_VERSION=1.9.671 8. Confirmed that it was actually using this to build Planck by seeing WARNING: Use of undeclared Var cljs.js/lib at line 450 out/cljs/js.cljs which is in that version of ClojureScript

mfikes02:07:05

Gist of the above in case it flys into the Slack ether: https://gist.github.com/mfikes/8be162fd730774e11990255345ee1127

mfikes02:07:42

Here is a subsequent successful Travis build, where I triggered it to build using ClojureScript 1.9.660 via the REST API: https://travis-ci.org/mfikes/planck-1/builds/251614094

mfikes02:07:50

Perhaps an expedient alternative to having individual cljs-oss projects then post the results somewhere, would be for us to instead have a main page that hosts a bunch of those Travis build status images, one per OSS project; these things: https://api.travis-ci.org/cljs-oss/planck.svg?branch=master

mfikes02:07:51

For the purposes of testing this in the actual cljs-oss org, I went into GitHub and granted access to cljs-oss to the Travis CI application

mfikes02:07:40

Triggered Planck in there and now you can see it https://travis-ci.org/cljs-oss

juhoteperi13:07:38

Hmm, I'm having problems getting react-dom/server work with cljs master

juhoteperi13:07:52

Uncaught Error: Undefined nameToPath for module$home$juho$Source$reagent$node_modules$react_dom$server

juhoteperi13:07:13

server.js file exists and is converted to Closure module

juhoteperi13:07:53

cljs_deps.js refers to this as reagent.dom.server dependency:

goog.addDependency("../reagent/dom/server.js", ['reagent.dom.server'], ['reagent.impl.util', 'reagent.interop', 'reagent.ratom', 'module$home$juho$Source$reagent$node_modules$react_dom$server', 'cljs.core', 'reagent.impl.template']);

juhoteperi13:07:44

but cljs_deps.js doesn't contain line that would provide react-dom/server

dnolen14:07:19

@juhoteperi hrm at least when running António’s tests on master I see that exact line

dnolen14:07:50

(let [{:keys [inputs opts]} {:inputs (str (io/file “src” “test” “cljs_build”)) :opts {:main ‘npm-deps-test.string-requires :output-dir “out” :optimizations :none :npm-deps {:react “15.6.1” :react-dom “15.6.1"} :closure-warnings {:check-types :off :non-standard-jsdoc :off}}}] (build/build (build/inputs (io/file inputs “npm_deps_test/string_requires.cljs”)) opts))

dnolen14:07:13

@juhoteperi run this at the REPL on master, and check out and you’ll see what I mean

dnolen14:07:55

@mfikes looking (also you have two patches that need rebasing :)

juhoteperi14:07:43

@dnolen Yes, works there... Hmm, the line is present on clean build in Reagent, but not after recompile.

dnolen14:07:19

here - trying that snippet again to see what happens

dnolen14:07:58

still there after a multiple runs

juhoteperi14:07:02

Running the snippet again works, perhaps this is something caused by lein/figwheel or something

dnolen14:07:00

@mfikes looks good, can you update the PR, than can make nit edits?

dnolen14:07:59

also I reverted the changes we made to the Node.js bootstrap in January - just not worth the hassle

mfikes14:07:53

@dnolen The aget PR is currently updated (I squashed onto it). So, it could be merged in and then de-nitted, or whatever approach makes sense.

mfikes14:07:27

All, cool, saw your "double employs" nit. Fixed.

dnolen14:07:54

@mfikes I’m also wondering if we should just do the check ?

dnolen14:07:16

i.e. add safe-aget which does type check, bounds check.

dnolen14:07:55

under advanced we aset macro does the old thing

dnolen14:07:19

since higher order aset can’t expected to be fast anyway

mfikes14:07:33

Oh. OK. I don't think I've seen code that switches when in :advanced. That makes sense though.

dnolen14:07:33

fn aset can just do these asserts when the compiler flag is set

dnolen14:07:33

can’t use analyzer.api here of course you can see how it’s done

dnolen14:07:13

@mfikes it will also be interesting to see what the perf impact will be - I wouldn’t be surprised if the checks are negligible under JSC if we always did them even at runtime

mfikes14:07:44

I think I'm following what you are suggesting.

mfikes14:07:19

Is safe-aget just a macro? Or also a fn?

dnolen14:07:31

writing a ticket to clarify.

mfikes14:07:09

:advanced could, over time, take on ClojureScript specific meaning of "elide expensive runtime checks" (reminds me of :elide-asserts)

darwin14:07:01

btw. having two versions of (macro) code depending on dev/advanced build setting could make sense in other scenarios as well, I mean in dev mode the emitted code could be more verbose with some useful intermediate state captured in well-named locals - this way when paused on a breakpoint user could see more useful stack frames - but this is really just a pie-in-the-sky idea

mfikes14:07:28

Tangentially related: If we ever have the std lib spec'd an interesting situation occurs for fns that also have macros. (The macros take over and defeat the ability to spec the fn when used in operator position.)

mfikes14:07:18

Yeah, like -g for C compilers, @darwin

mfikes14:07:45

(non-optimized, with symbols)

darwin14:07:39

@mfikes, not familiar with that, but in general, in dev-mode code should be generated for developer happiness, in advanced mode for runtime/performance 🙂

darwin14:07:01

but I understand that this could be very tricky, because the behaviour could unintentionally diverge and performance profile could be way off between those modes

mfikes14:07:28

In an alternative world (where code size isn't important), simple code that gets JITd aggressively in the target environment is nice too. Perhaps in a decade or two, everything will be clean and self-hosted. Feels like we are still in an era similar to when developers tried to keep their assembly compact.

juhoteperi14:07:55

@dnolen Re react-dom/server problem, try (def state (cljs.analyzer.api/empty-state)) and passing state as third param to build. This way I can reproduce the problem.

juhoteperi14:07:22

https://github.com/clojure/clojurescript/blob/master/src/main/clojure/cljs/analyzer.cljc#L722 analyzer uses this to check if missing-js-modules should be added to namespace map of the ns which has require to e.g. react-dom/server

juhoteperi14:07:37

And during the second build doesn't happen as module is already present in the compiler state

darwin14:07:04

@mfikes let’s get back to cljs-oss, I looked at your work, it looks good, I would still propose not to create new .travis.yml files, but reuse the maintained ones in libraries themselves, also to your note “where to store results”, I would propose to have a special branch on main cljs-oss mono-repo which would collect all the results in commit messages (or in generated files markdown inside that branch)

darwin14:07:06

do you want me to go ahead and prepare proposal for that skeletal mono-repo which would do the work? I can spend some time on it right now @juhoteperi @mfikes

darwin14:07:25

I will do it just as a proposal, if you don’t like it, no big deal

dnolen14:07:45

I think this a pretty simple thing to do

dnolen14:07:07

just having bounds check over JS for dev is huge

mfikes14:07:09

@darwin Yeah, sure. My work with cljs-oss/planck was just to explore. If there are refinements we can make to simplify, that makes sense.

dnolen14:07:41

@mfikes it also gives your post a lot more power - since you can immediately leverage the points made

mfikes14:07:55

Yeah. I'll take a look at that ticket

dnolen14:07:18

@mfikes seems 2192 still needs a rebase

mfikes15:07:00

Interesting. I rebaselined it, and was wondering if the rebaseline was even needed. Will try to repro what you are seeing.

mfikes15:07:27

@dnolen The patch in 2192 is applying cleanly for me on master. Hrm.

dnolen15:07:30

oh sorry PEBCAK

dnolen15:07:07

@mfikes or not - I can’t get it to work

dnolen15:07:35

the patch I downloaded

dnolen15:07:39

From: Mike Fikes <[email protected]> Date: Sat, 8 Jul 2017 15:56:10 -0400

dnolen15:07:46

that doesn’t seem right? (the date)

dnolen15:07:19

yes time stamp for v2 patch is same as the first one

mfikes15:07:16

I think this is because it is keeping my original "author" date when I re-baseline. I'm making a fresh clone of master and checking if I can apply.

mfikes15:07:06

@dnolen I think I see what is going on

mfikes15:07:22

It is the DOS newlines causing whitespace error warning when applying

mfikes15:07:08

When I apply it I get:

/Users/mfikes/Downloads/CLJS-2192-v2.patch:112: trailing whitespace.
and another similer warning

dnolen15:07:50

@juhoteperi I’m curious why that js-module-exists? is present at all when computing missing-js-modules in the analyzer

dnolen15:07:08

@juhoteperi can you try removing it on your end and see if it fixes the issue?

mfikes15:07:11

If I vi the patch file I see ^M characters at the end of the line where it is patching the script/test.ps1 file.

dnolen15:07:48

this is what I see:

mfikes15:07:42

@dnolen Yep, I can repro that with git am but get the whitespace warnings with git apply. Digging. I'll try to create a patch using exactly the instructions we have on our site.

juhoteperi15:07:45

@dnolen Doesn't work. The first time missing-js-modules gets value #{react-dom/server}, next time #{module$home$juho$Source$clojurescript$node-modules$react-dom$server}

dnolen15:07:29

@juhoteperi hrm how does that happen?

juhoteperi15:07:31

Looks like the require values in analyzer are already resolved to the module name

dnolen15:07:20

I think I know, let me try something

mfikes15:07:12

@dnolen I am able to apply the patch by using --ignore-whitespace. I think the reason is that it patches a mixture of Unix and DOS files. Here is a transcript showing a fresh clone, etc.

$ ls
CLJS-2192-v2.patch
$ git clone 
Cloning into 'clojurescript'...
remote: Counting objects: 33824, done.
remote: Compressing objects: 100% (26/26), done.
remote: Total 33824 (delta 7), reused 23 (delta 6), pack-reused 33790
Receiving objects: 100% (33824/33824), 12.34 MiB | 11.39 MiB/s, done.
Resolving deltas: 100% (16156/16156), done.
$ cd clojurescript/
$ git am ../CLJS-2192-v2.patch
Applying: CLJS-2192: Add ChakraCore testing facilities
error: patch failed: script/test.ps1:7
error: script/test.ps1: patch does not apply
Patch failed at 0001 CLJS-2192: Add ChakraCore testing facilities
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
$ git am --abort
$ git am --ignore-whitespace ../CLJS-2192-v2.patch
Applying: CLJS-2192: Add ChakraCore testing facilities

mfikes15:07:57

Oh. no don't do the above @dnolen as it messes up the DOS file.

mfikes15:07:33

OK @dnolen, empirically it appears the --keep-cr option works. It results in the same warning that git apply warns about, but results in the patch being applied, and the DOS file (`script/test.ps1`) having the DOS line endings being properly handled in the patched result.

$ git am --keep-cr ../CLJS-2192-v2.patch
Applying: CLJS-2192: Add ChakraCore testing facilities
.git/rebase-apply/patch:107: trailing whitespace.
    @{ env="NASHORN_HOME"; name="Nashorn"; cmd={ & "$env:NASHORN_HOME\jjs" $testjs } },
.git/rebase-apply/patch:108: trailing whitespace.
    @{ env="CHAKRACORE_HOME"; name="ChakraCore"; cmd={ & "$env:CHAKRACORE_HOME\ch" $testjs } }
warning: 2 lines add whitespace errors.

mfikes16:07:07

It looks like the option was added to git to handle funky Mail line endings. But it works for this mixed DOS/Unix patches. FWIW: https://stackoverflow.com/a/24178810/4284484

dnolen16:07:37

@mfikes got the patch to apply thanks, testing now

mfikes16:07:14

Cool. At least the patch was not in EBCDIC.

mfikes16:07:08

This site PR accompanies the ChakraCore patch: https://github.com/clojure/clojurescript-site/pull/109

dnolen16:07:34

great thanks

dnolen16:07:47

@mfikes pretty cool to see the ChakraCore tests

dnolen16:07:07

@mfikes later - it would be nice to make our own Are We Fast Yet page on cljs-oss

mfikes16:07:20

Yes. Makes me feel like ClojureScript can now claim support for that VM.

dnolen16:07:25

and publish engine numbers for our own kinds of benchmarks

dnolen16:07:14

it could possibly be a nice reference point for other functional languages targeting JS

dnolen16:07:23

@juhoteperi I think I found the fix

dnolen16:07:25

we just need to dissoc :js-module-index when collecting sources so that build becomes idempotent again

anmonteiro17:07:46

So problem with tests or problem with the npm-deps feature?

juhoteperi17:07:56

@dnolen That would fix this. I don't know enough about analyzer side to know if that will break something else.

dnolen17:07:43

@juhoteperi pretty sure it cannot - this is only in the sources collecting pass

dnolen17:07:50

(there’s no compilation here at all)

dnolen17:07:21

the problem is that if you include the JS module index parse-ns become non-idempotent

anmonteiro17:07:22

Oh, I know the bug

anmonteiro17:07:50

We call -find-sources twice

anmonteiro17:07:15

And the 2nd time we've already processed modules

anmonteiro17:07:46

So there's no point missing-js-modules being there

dnolen17:07:08

yes the second time

dnolen17:07:17

but my question is do we need to call it twice anyway?

juhoteperi17:07:24

I don't think that causes the problem with cljsbuild/figwheel/etc

anmonteiro17:07:38

I don't know if we do

anmonteiro17:07:53

It was already like that and I didnt wanna touch it

dnolen17:07:16

ok then I think dissociating the :js-module-index in the first one is fine

dnolen17:07:25

and we can revisit

anmonteiro17:07:31

Dissocing from what?

dnolen17:07:41

the compiler state

dnolen17:07:49

the first -find-sources can’t possibly use that

anmonteiro17:07:12

I think we remove the ones that we know about in parse-ns

anmonteiro17:07:20

Perhaps that's the bug

dnolen17:07:35

it’s actually way more annoying 🙂

anmonteiro17:07:55

I'm on phone so just taking guesses from memory

dnolen17:07:57

but also maybe necessary

dnolen17:07:16

@anmonteiro anyhow I have a quick fix that I don’t think can cause problems

dnolen17:07:42

+ I’m adding test so we can catch this issue regardless of the actual solution

anmonteiro17:07:04

Awesome, thanks

anmonteiro17:07:42

I feel like CLJS test coverage went way up too for this release

anmonteiro17:07:03

Maybe we could also highlight that in one of the posts

darwin17:07:15

just created this repo with some notes and docker wrapping working @mfikes, @juhoteperi: https://github.com/cljs-oss/canary

anmonteiro17:07:41

Also hoping to get CLJS-1428 in, that'll be huge for bridging yet another gap between Lumo and Planck

darwin17:07:55

the tool still needs implementation, but it should be pretty easy: https://github.com/cljs-oss/canary/blob/master/runner/src/cljs_oss/runner.clj#L27

darwin17:07:41

will be back in 2hrs or so

anmonteiro17:07:02

@dnolen maybe this is adding unnecessary churn, but hasn't the Node.js bootstrapping changed over time in Closure? https://github.com/google/closure-library/blob/master/closure/goog/bootstrap/nodejs.js

anmonteiro17:07:33

Are we up to date with that or can we learn something from it?

dnolen17:07:54

@anmonteiro the only difference is the sync load thing

dnolen17:07:01

which is kind of interesting, for :modules support under Node.js I guess - though that seems strange to me

dnolen17:07:07

but maybe useful for testing

dnolen17:07:11

@juhoteperi try master when you can

dnolen17:07:28

@anmonteiro landed CLJS-1428

juhoteperi17:07:22

@dnolen Still having problems: No such namespace: module$home$juho$Source$reagent$node-modules$react-dom$server .....

dnolen17:07:43

@juhoteperi but your cljs_deps.js looks OK?

juhoteperi17:07:06

But I think it won't write that as that exception is thrown

dnolen17:07:25

I don’t know you mean ^

juhoteperi17:07:48

Exception is thrown before Cljs compiler writes the cljs_deps.js file

dnolen17:07:25

@juhoteperi we have a test that builds twice - so I need some clarity as to what you’re observing

dnolen17:07:38

we use the same complier state for both builds

juhoteperi17:07:02

I have an idea, testing

dnolen17:07:04

make sure you don’t have anything weird going on here like a target directory if you’re testing in the repo

anmonteiro18:07:23

I could swear that was passing for me?

anmonteiro18:07:39

@mfikes that’s probably a bogus failure

anmonteiro18:07:56

assumes you set the output-dir to whatever it’s set when running script/test

juhoteperi18:07:18

Maybe I understand now: missing-js-modules are collected after -find-sources is called, that will only collect the sources from build source files/dirs etc. Not files from classpath.

anmonteiro18:07:08

@juhoteperi should we be doing more?

juhoteperi18:07:08

Except in Reagent src is in cljsbuild source-paths so it should be included in -find-sources... Not sure yet.

anmonteiro18:07:33

^ oh, self-parity tests

dnolen18:07:58

@juhoteperi @anmonteiro oh yeah - that’s going to be a problem

dnolen18:07:25

that means :npm-deps won’t work correctly for packages libs (JARs) with sources w/ string requires

juhoteperi18:07:02

And projects where Cljs inputs is a single file and other files are loaded from classpath.

dnolen18:07:28

so I think the process-js-modules needs to come after add-dependency-sources

anmonteiro18:07:10

^ not sure how this is going to play with chakracore

anmonteiro18:07:26

oh, nevermind. we’re not running tests on windows

juhoteperi18:07:47

problem will be the add-implicit-options, but probably the foreign-libs part from that can also be moved after add-dependency-sources

anmonteiro18:07:42

yeah, the problem is adding the implicit options

anmonteiro18:07:52

I agree we should be able to do that

juhoteperi18:07:54

Testing this now

dnolen18:07:54

does someone want to tackle or should I take a look?

juhoteperi18:07:22

I can work on this, for now I have problems and I'm unsure on how to reproduce those other than with Reagent

anmonteiro18:07:27

happy to do it as well, my only problem is I wouldn’t know how to write a test

dnolen18:07:05

@anmonteiro just put a dep on the classpath that you require

anmonteiro18:07:16

and that dep has a string require

dnolen18:07:26

in npm-deps-test require a different ns on classpath that does the string require

anmonteiro18:07:48

trying that now. @juhoteperi or do you wanna fix this?

juhoteperi18:07:51

@anmonteiro I'm already working on this

anmonteiro18:07:00

you go for it!

anmonteiro18:07:22

hrm I think this is a bug in the REPL:

cljs.user=> (require 'clojure.string :reload-all)
#{"foo.core" "goog.dom.NodeType" "cljs.spec.gen.alpha" "goog.array" "clojure.walk" "goog.reflect" "goog.asserts" "goog.object" "clojure.string" "goog.math.Long" "cljs.repl" "goog.string" "goog.debug.Error" "cljs.spec.alpha" "goog.math.Integer" "cljs.core" "cljs.pprint" "goog.string.StringBuffer"}

anmonteiro18:07:28

shouldn’t we be returning nil?

mfikes18:07:49

Already patch attached for this @anmonteiro in one of the recent JIRAs. Digging it up.

juhoteperi18:07:59

I think I now have string requires in classpath files working, and now I can reproduce the same problems as with Reagent. If file that uses string requires is modified -> recompile doesn't find the JS file. No files modified -> cljs_deps.js is missing the line for JS file.

juhoteperi19:07:22

Aand works with Reagent

juhoteperi19:07:12

blah, noticed a typo in one of the comments

juhoteperi19:07:23

no... it is correct 😄

anmonteiro19:07:50

my current understanding is that we need to bind ana/*analyze-deps* to false because we’re computing & loading them manually

anmonteiro19:07:03

does this assumption break anything?

anmonteiro19:07:51

I don’t think it breaks anything

juhoteperi20:07:21

What is best way to provide process.env.NODE_ENV currently?

juhoteperi20:07:23

I have process.env ns with goog-define but looks like React code is being loaded before that ns

dnolen20:07:27

@juhoteperi had to provide a shim ns process/env.cljs in the past

dnolen20:07:11

@anmonteiro I think that may be correct re 1764

anmonteiro20:07:32

@juhoteperi if it’s just for testing try putting 'process.env in preloads

anmonteiro20:07:42

@dnolen yeah I’m piloting the REPL with cljs-1764 locally and it doesn’t seem to break anything

juhoteperi20:07:55

Okay, with preloads process.env works

dnolen20:07:37

@anmonteiro yeah I think it’s ok, add-dependencies puts everything in dep order

dnolen20:07:29

gotta run for now - but this is pretty awesome stuff 🙂

mfikes20:07:37

It turns out we are passing array-like JavaScript objects to aget (the primary example being arguments).

dnolen20:07:17

ah right, goog.array.isArrayLike is a thing

dnolen20:07:37

but I would test array? first for happy path

mfikes20:07:39

Cool. Thanks. For now I'll try using that and array? to avoid tripping up

mfikes20:07:07

(I'm not at the point of prod code, just trying to make it through some of the mud.)

dnolen20:07:28

actually it’s at top level

anmonteiro20:07:49

@juhoteperi the tests you added, are they broken without the fix?

anmonteiro20:07:38

did some more extensive testing of the REPL patch (CLJS-1764) locally in a largeish project

anmonteiro20:07:43

couldn’t find any edge cases

juhoteperi21:07:58

@anmonteiro Yes, the were broken with the added classpath file

anmonteiro21:07:02

I’m asking because I thought it wouldn’t be in the classpath

anmonteiro21:07:31

like, I don’t think src/test/cljs_build is ever added to the classpath, and we specify only 1 input https://github.com/clojure/clojurescript/blob/master/src/test/clojure/cljs/build_api_tests.clj#L241

anmonteiro21:07:45

just trying to prevent a regression

juhoteperi21:07:19

src/test/cljs is in classpath

juhoteperi21:07:29

The build input file is under src/test/cljs_build, but new file is under src/test/cljs/

anmonteiro21:07:21

missed that, thanks

anmonteiro21:07:18

it was there all along for me to see… string_requires_in_classpath.cljs

anmonteiro21:07:21

sorry for the noise

mfikes21:07:57

@anmonteiro Dunno if you are interested in https://dev.clojure.org/jira/browse/CLJS-2204 (it may have been behind the other failure)

anmonteiro21:07:18

hrm I guess the fix to that is to just include src/test/cljs_build in the lein classpath for testing

anmonteiro21:07:04

I don’t think there’s any problem in doing that

anmonteiro22:07:21

@mfikes added a patch to CLJS-2204, can you make sure it works for you?

mfikes22:07:29

Thanks. Will do now.

anmonteiro22:07:34

both lein test and script/test pass for me

juhoteperi22:07:33

Hmmmm, I don't think is correct solution. That folder is supposed to be outside of classpath.

juhoteperi22:07:24

lein test should work without changes

anmonteiro22:07:38

lein test works

anmonteiro22:07:42

the problem is script/test

anmonteiro22:07:54

because it tries to compile that npm-deps thing

anmonteiro22:07:19

maybe the right solution is to create yet another directory under src/test that lein knows about but script/test doesn’t

juhoteperi22:07:25

Oh right, because it tries to complie any files under src/test/cljs

juhoteperi22:07:31

it was not a good idea to put that file there

juhoteperi22:07:03

New directory is a good solution

anmonteiro22:07:54

I’ll modify my patch to do that

anmonteiro22:07:54

revised the patch to include a new cljs_cp directory

anmonteiro22:07:23

maybe we could even migrate some things under src/test/cljs that are not used by script/test to this folder

anmonteiro22:07:00

pushing it even further, remove src/test/cljs from the lein test classpath

anmonteiro22:07:14

if there are no common things between script/test and lein test