Fork me on GitHub
#cljs-dev
<
2018-01-12
>
juhoteperi08:01:43

I got the test script working, I had to remove lib/ which had some old jars, which bootstrap script didn't replace or remove

juhoteperi08:01:58

Btw. deps between bootstrap script, project.clj and pom.template.xml don't match. Bootstrap uses Clj 1.9.0-alpha17, project.clj 1.9.0, pom 1.8.0. Pom is also missing core-specs-alpha dependency.

Alex Miller (Clojure team)14:01:27

fyi, clojure 1.9 depends on core.specs.alpha, so you will get that transitively if depending on 1.9

anmonteiro12:01:59

perhaps tools.deps is a good way to unify all these ^

anmonteiro12:01:06

in a single place

juhoteperi12:01:38

Not sure if it will help with pom.xml? But yes, will probably help with bootstrap and lein

mfikes12:01:31

@juhoteperi If you have a deps.edn you can generate a pom.xml using clojure -Spom

juhoteperi12:01:06

Lein could generate pom.xml also, I think there is some reason why clojurescript uses separate file

mfikes12:01:18

(Here is the ClojureScript deps.edn I was messing with. https://gist.github.com/mfikes/9485eaded14ae100e1174586446aeb3d)

juhoteperi12:01:49

I think the pom.template.xml is the master for Cljs currently

mfikes12:01:54

(The only testing I did with that was to see if I could depend on ClojureScript from another project that also uses deps.edn.)

juhoteperi12:01:56

So it will require plugins etc. for builds

mfikes13:01:01

Ahh, right.

juhoteperi13:01:54

@dnolen If you apply patches today, I think the Closure update patch is OK now: https://dev.clojure.org/jira/secure/attachment/17621/CLJS-2389-5.patch

rauh15:01:37

@dnolen I can provide patches for any of my last few tickets, just let me know if you're interested and I'll produce them. The chunking for small vectors + faster reduce for them will likely make a lot of code faster.

dnolen15:01:12

@rauh patches welcome, though I probably won’t get a chance to look at those particular ones today

rauh15:01:44

@dnolen Sounds good, just to summarize: 1. Add -reduce for ChunkedCons 2. Make seq-reduce check for chunked 3. Add IChunkedSeq for IndexedSeq. All three ok?

dnolen16:01:29

@rauh do these changes parallel what’s be done in Clojure? that should be in the ticket - whether this is novel work

dnolen16:01:40

also benchmarks of course

rauh16:01:14

Benchmarks are already included, about 4x-5x faster for the chunking and 5-10x faster for seq-reduce. The seq-reduce is already done in Clojure, not sure if there has ever been a ticket for it. The chunking was also never done in Clojure since (seq [1 2 3]) has always returned a ChunkedCons in Clojure.

rauh16:01:08

Though, the approach would a different one to Clojure: I think making IndexedSeq a chunk makes sense since we (in CLJS) use IndexedSeq a lot more than Clojure

rauh16:01:29

Eg: (apply (fn [& a] (type a)) [1 2 3 4 8]) returns ChunkedCons in Clojure but IndexedSeq in CLJS.

dnolen16:01:05

I would not put all this into one ticket

dnolen16:01:23

2. sounds obvious since already done in Clojure

dnolen16:01:40

what’s the story on 1? 3 needs to be assessed more closely

dnolen16:01:19

definitely need to make sure these are separate tickets so it’s easier to go through

rauh16:01:49

1. Is also done in Clojure:

rauh16:01:04

2. Is also done in Clojure. But that's the only trickier part where I'd like a review. Need to be careful to not end up blowing the stack

rauh16:01:36

3. Is different to Clojure since IndexedSeq's are more common for us in CLJS

rauh16:01:01

I'll create a separate ticket for implementing the chunkedcons reduce.

dnolen18:01:51

@juhoteperi did you run ./script/test?

juhoteperi18:01:31

I only had JSC installed but that passed tests

dnolen18:01:59

tests are failing for me

dnolen18:01:01

builds/out-adv/core-advanced-test.js:3167: ReferenceError: module is not defined

dnolen18:01:58

same error on all the JS runtimes I have installed

dnolen18:01:52

seems to be coming from lodash exports which makes sense?

juhoteperi18:01:07

Oh didn't notice that

juhoteperi18:01:26

maybe related to this ERROR: JSC_JSON_UNEXPECTED_TOKEN. Unexpected JSON token at /home/juho/Source/clojurescript/node_modules/lodash/package.json line (unknown line) : (unknown column) ERROR: JSC_PARSE_ERROR. Parse error. Semi-colon expected at /home/juho/Source/clojurescript/node_modules/lodash/package.json line 2 : 9

dnolen18:01:36

I don’t think so

dnolen18:01:39

I don’t see that

dnolen18:01:53

module appears in lodash and no JS runtime is going to know what that is

dnolen18:01:03

except Node.js

juhoteperi18:01:36

Closure should have rewritten those references

dnolen18:01:40

so are these module export lines not getting erased by Closure

dnolen18:01:45

yeah so I’m not seeing that here

juhoteperi18:01:11

Closure processing doesn't work with cljsc

dnolen18:01:39

./script/test :lang-in is :es6 and :lang-out is :es5

dnolen18:01:42

is this right?

juhoteperi18:01:48

oops, I have two Closure compiler jars on lib/

dnolen18:01:46

we should probably rm lib/ when running script/bootstrap

juhoteperi18:01:25

Yes, second time I missed this

juhoteperi18:01:53

Huh still doesn't pass. I remember seeing the successful output.. hm

juhoteperi18:01:56

lein test :only cljs.build-api-tests/test-npm-deps

juhoteperi18:01:05

cat /tmp/npm-deps-test-out/node_modules/lodash/toLength.js

juhoteperi18:01:10

using lein test the processing works

juhoteperi18:01:18

what can cause the difference with cljsc?

dnolen18:01:27

lein test is working for me

dnolen18:01:31

but ./script/test isn’t

dnolen19:01:22

@juhoteperi seems to me there’s probably just a bug with the patch

dnolen19:01:38

when you look at builds dir you see all the lodash files

dnolen19:01:42

but they are all wrong

juhoteperi19:01:48

Yes, but I don't understand what difference causes with lein and test script

dnolen19:01:49

like the pass didn’t work

dnolen19:01:57

I don’t think that’s important

dnolen19:01:05

I would just look for the bug

juhoteperi19:01:24

But how can I look for the bug when it works already using lein

dnolen19:01:35

I don’t know what that has to do with finding the bug

dnolen19:01:42

who cares about lein

dnolen19:01:57

drop some debugging statements in there and run ./script/test

dnolen19:01:01

that’s what I’m doing

juhoteperi19:01:06

Do you see the error about Error: Can't resolve './calculator_amd'?

dnolen19:01:29

that’s always been there

dnolen19:01:36

or rather it’s been there for a long time

dnolen19:01:19

@juhoteperi when calling ./script/test, convert-js-modules is never called, this doesn’t seem right

juhoteperi19:01:32

It is called for me

juhoteperi19:01:46

How are you testing? Println? Output is redirected to the file

juhoteperi19:01:18

Closure parse error about lodash package.json stops the Closure processing for me

juhoteperi19:01:56

We should throw exception in report-failure if there was errors, output will be broken in that case

dnolen19:01:31

ah right, forgot I need to use util/debug-prn

dnolen19:01:45

@juhoteperi ah so the issue is that we’re blindly processing package.json as JS

dnolen19:01:04

we need it as an input of course for Node resolution, but we shouldn’t be trying to process it

juhoteperi19:01:56

Hmph, no, Closure should be able to process it

juhoteperi19:01:16

Because requiring package.json to read the package information works in CJS

dnolen19:01:21

but it probably doesn’t work for ES6?

dnolen19:01:29

we’re applying all these passes hoping they will work

dnolen19:01:25

@juhoteperi yeah I’m not sure I believe Closure will process these inputs or should

dnolen19:01:40

you only need package.json for resolution, it’s never going to be part of the build

juhoteperi19:01:20

exports.version = require('./package.json').version;

juhoteperi20:01:32

And Closure also needs to read the package.json again for dependencies

juhoteperi20:01:53

Our package information is only used to select which files to pass into Closure

juhoteperi20:01:58

They do the same again

juhoteperi20:01:55

(They read the main property information from package.json files at least)

juhoteperi20:01:07

And there is a pass to rewrite them into JS modules, which is called during parse

dnolen20:01:31

@juhoteperi so then what step are we missing?

dnolen20:01:37

since parse fails for us?

juhoteperi20:01:33

I have Lodash processing working in another env so I'm still thinking there is some difference with bootstrap or test script

juhoteperi20:01:08

Bootstrap uses different Closure jar so I wonder how that is different

dnolen20:01:08

I just don’t believe that’s relevant

dnolen20:01:57

oh … it doesn’t use the unshaded jar

dnolen20:01:30

maybe this is a good excuse to try deps.edn, the bootstrap script is just a pile of workarounds for grabbing the Maven deps we need and managing the classpath

mfikes20:01:43

Perhaps the effort put into making deps.edn work would pay for itself in terms of making it easier for community members to try MASTER without even (manually) cloning it.

mfikes20:01:43

I suppose to really reach that nirvana, end users would need to be using deps.edn themselves…

mfikes20:01:14

I mean… the description on this page becomes almost nothing: https://clojurescript.org/community/building

dnolen20:01:04

@alexmiller the basic deps.edn docs don’t cover what to do if your source paths aren’t under src?

dnolen20:01:43

or is the test source section suppose to cover that?

Alex Miller (Clojure team)20:01:38

^^ does that answer your question?

dnolen20:01:50

@alexmiller clj -R:test bin/cljsc.clj "..."

dnolen20:01:58

so does "..." not get passed as CLI args?

Alex Miller (Clojure team)20:01:51

this is really a clojure.main question - I think you have to use -m if you want to invoke -main with args

Alex Miller (Clojure team)20:01:04

using a script will just load and eval the script

dnolen20:01:52

so just add -m?

Alex Miller (Clojure team)20:01:00

does cljsc.clj have a -main?

Alex Miller (Clojure team)20:01:23

then I don’t think that will work

dnolen20:01:43

it’s just a script that looks at *command-line-args*

dnolen20:01:05

@alexmiller the error is a bit cryptic

dnolen20:01:09

Exception in thread "main" java.io.FileNotFoundException: {:optimizations :advanced ...

mfikes20:01:10

@dnolen I have a similar script that uses *command-line-args* successfully https://github.com/mfikes/coal-mine/blob/master/script/build#L4

dnolen20:01:20

but I passed a script as first arg?

mfikes20:01:23

But I invoke it directly (or using hashbang)

mfikes20:01:10

I can’t repro what you are seeing @dnolen

$ cat foo.clj
(prn *command-line-args*)
$ clj foo.clj 1 2 3
("1" "2" "3")

Alex Miller (Clojure team)20:01:02

yeah, looking at the code it seems like command-line-args should be set when using the script

Alex Miller (Clojure team)21:01:34

if I add (prn *command-line-args*) to the top of bin/cljsc.clj I see the args passed with clj bin/cljsc.clj 1 2

Alex Miller (Clojure team)21:01:24

if your “…” is multiple options with spaces between then maybe that bash quoting is failing you

Alex Miller (Clojure team)21:01:15

(or maybe clj itself is deficient in that regard, but I’m pretty sure it is ok)

dnolen21:01:20

I it figured out

dnolen21:01:22

clj is working

dnolen21:01:11

now hitting other issues, but @juhoteperi no change, so it doesn’t look like it has anything to do with the Closure Compiler dep

dnolen21:01:05

@alexmiller clj wouldn’t affect picking up data_readers.cljc files right?

Alex Miller (Clojure team)21:01:34

only insofar as it affects your classpath

Alex Miller (Clojure team)21:01:41

clj -Spath will dump your classpath if that helps

dnolen21:01:59

@alexmiller any reason clj -R:test -Spath isn’t showing my :test alias extra paths?

Alex Miller (Clojure team)21:01:17

because you need -C:test

Alex Miller (Clojure team)21:01:20

-R affects resolve-deps, -C affects building the classpath. there is a ticket to merge these and we may do that.

dnolen21:01:05

@juhoteperi yeah I got tests running via clj and there is no change, so the fact that this is somehow working under lein isn’t telling us much

dnolen21:01:06

@alexmiller right makes sense, I just didn’t understand the distinction

juhoteperi21:01:13

I'll continue looking into this and will report when I find the fix