Fork me on GitHub
#cljs-dev
<
2016-09-29
>
jasoncourcoux12:09:16

Hi! I was wondering if anyone would mind if I picked up http://dev.clojure.org/jira/browse/CLJS-1195 as a first contribution? I've signed the CA, so assuming you're happy, and no one else is already looking at it - is this the right place to ask about getting the correct jira permissions to assign to myself?

dnolen13:09:37

Might want to assign to yourself

jasoncourcoux13:09:38

@dnolen Thanks! Is that something I'll need permissions for - and are you the right person to ask for those?

dnolen13:09:55

one thing we should think about is whether we should just give up on maintaining cljs.reader now that we have tools.reader as dep. Perhaps we should just refer to tools.reader for that functionality?

dnolen13:09:33

yeah it has an edn reader ns ...

dnolen13:09:52

and I already perf tuned a lot of this code when I did bootstrap

thheller13:09:51

@dnolen I don't understand your comment about require and data literal files? what do they have to do with it?

anmonteiro14:09:41

@thheller I think in CLJS we’ll have to require the namespaces that declare a data reader unlike in Clojure because of dependency order

anmonteiro14:09:55

it’s how I interpret David’s statement

thheller14:09:34

@anmonteiro not sure what you mean

anmonteiro14:09:18

@thheller my understanding about the ClojureScript compilation process is that we need to do topological sort on the dependencies

anmonteiro14:09:39

in order for dependents to see populated analysis already

dnolen14:09:47

@thheller there are just many cases where Clojure files don’t have namespaces

dnolen14:09:51

this isn’t about scripts

dnolen14:09:04

I’m not going to say anything more about that ticket - nor take any more feedback

dnolen14:09:11

it’s slated for implementation whoever gets around to it

thheller14:09:45

@dnolen if you say it is useful it probably is ... just trying to understand

dnolen14:09:00

I’m not going to explain it anymore sorry

dnolen14:09:15

@anmonteiro data literal files aren’t any different from any other ns without an ns form

dnolen14:09:31

if you have records in some other ns that you’re going to construct then you will require

dnolen14:09:43

if everything is in the file, then load order doesn’t matter anyway

thheller14:09:48

I always thought data_readers.clj was just unfortunately not called data_readers.edn but if they can contain require i guess that is a viable use

anmonteiro14:09:45

@dnolen I’m starting to look at what might be needed to do the suggested implementation, but I’m stuck on when should the sub-ns be generated

anmonteiro14:09:05

because a file might have several requires e.g.: require, require-macros ,etc

dnolen14:09:29

@anmonteiro right so that was implied with that I was saying yesterday a bit

dnolen14:09:48

once you don’t see an ns form and you see something else you know the ns must be cljs.user.FOO

dnolen14:09:03

then you consume only ns special forms until you hit something which isn't

anmonteiro14:09:35

I can understand that, sure. but then we hit the case of several files might not have ns declarations

dnolen14:09:45

I don’t see how that is a problem

anmonteiro14:09:59

if both declare a var with the same name?

dnolen14:09:10

@anmonteiro right so I think you’re missing my solution 🙂

dnolen14:09:33

so I have a file foo.cljs which doesn’t have an ns

dnolen14:09:16

this will generate namespace called cljs.user.$$generated$$_foo0 keyed to URI

dnolen14:09:34

this namespace will (def x 1)

dnolen14:09:41

then I have file bar.cljs w/o ns

dnolen14:09:58

this will generate namespace called cljs.user.$$generated$$_bar0 keyed to URI

dnolen14:09:48

so assuming the user did something to guarantee that foo.cljs comes before bar.cljs

dnolen14:09:01

how could I refer to x in bar?

dnolen14:09:22

this is possible because we’ll custom resolve cljs.user vars

dnolen14:09:39

x isn’t in bar, but we know all the user nses, we can find it

anmonteiro14:09:53

so in bar you’d just refer to x without requiring anything?

dnolen14:09:59

absolutely

anmonteiro14:09:03

because theoretically they’d be in the same cljs.user ns

dnolen14:09:23

so this is the big idea, the illusion of a big cljs.user ns, but in fact it’s just Closure namespaces

dnolen14:09:29

so there’s nothing to fix, nothing sort, nothing to do

anmonteiro14:09:59

so in the compiler environment we cram everything into cljs.user?

dnolen14:09:20

that’s the easy way, with each var pointing to it’s actual location

dnolen14:09:24

as we already do

anmonteiro14:09:37

“actual location” = filesystem?

dnolen14:09:39

x -> cljs.user.$$..

anmonteiro14:09:50

or rather, closure namespace

anmonteiro14:09:56

which might or might not map to a file

anmonteiro14:09:03

depending on optimization settings

dnolen14:09:22

yes more or less

dnolen14:09:34

the internals should be generic so this works for bootstrapped

dnolen14:09:48

but the first goal is just make this work for Closure dev and prod builds

anmonteiro14:09:09

so that clears some things up but I still have my initial question

anmonteiro14:09:21

what if the user declares x in foo and bar?

dnolen14:09:49

it should probably just over write like Clojure does

anmonteiro14:09:51

that’ll probably generate a compiler warning

dnolen14:09:58

we could optionally issue a warning

dnolen14:09:01

but this is secondary concern

anmonteiro14:09:19

right, now I get what I was missing

anmonteiro14:09:40

if a user has 2 files without namespace, they are assumed to be cljs.user

anmonteiro14:09:08

so it’s basically a user concern not to have duplicated names in there

dnolen14:09:15

just like Clojure

dnolen14:09:18

nothing new here 🙂

anmonteiro14:09:24

yeah, but I somehow was missing that 🙂

dnolen14:09:42

so generating a stable name for namespaces

dnolen14:09:14

perhaps it should be cljs.user.$$genns_$$FILENAMEFIRST_CHARS_HASH_OF_URI

dnolen14:09:20

or something like that - they need to be stable - not random

dnolen14:09:03

then Figwheel etc. all that stuff will work

dnolen14:09:06

there are probably some small corners to think through with respect to REPLs but it seems OK if we have the resolution stuff worked out internally

dnolen14:09:46

yeah getting the resolution stuff in the analyzer right will simplify everything else

anmonteiro14:09:36

definitely not a straightforward task, but looks very interesting

dnolen14:09:06

basically we want to completely hide the Closure namespace stuff behind the scenes

dnolen14:09:18

and present the illusion of a single cljs.user ns

jasoncourcoux16:09:23

@dnolen - sorry got dragged away for a bit - was the tools.reader comment earlier a hint in regards to the processing of command line arguments for Jira 1195? i.e. are you suggesting that the arguments would be passed as a string of clojure data which gets parsed by the reader, and therefore the implementation should look at making use of tools.reader, rather than say either some custom code/lib/java platform to parse standard unix style arguments? Or have I completely missed the point, and your comment was unrelated to this particular issue!

dnolen16:09:46

@jasoncourcoux no, unrelated, just a enhancement thought

jasoncourcoux16:09:58

Haha - okay, thought that was probably the case

dnolen16:09:39

@jasoncourcoux I bumped your permissions

jasoncourcoux16:09:40

I'll probably have several questions - just kind of throwing myself in as thought that's the best way to learn

anmonteiro16:09:51

@dnolen by “stop maintaining cljs.reader” do you want to convey we should just delete the namespace in a future release?

dnolen16:09:14

@anmonteiro no, just defer to tools.reader for the api

anmonteiro16:09:24

ah makes sense

dnolen16:09:39

meaning the ns remains but the functionality comes from tools.reader

anmonteiro16:09:52

I’m definitely in favor of that

dnolen16:09:02

maintaining that ns when Nicola keeps it very up-to-date is wasted effort

anmonteiro16:09:05

I got bitten by cljs.reader before and ended up using tools.reader anyway

anmonteiro16:09:59

@dnolen If I’m thinking correctly about the problem, require etc support in the analyzer will allow us to delete a good portion of the REPL special functions code, no?

dnolen16:09:30

@anmonteiro that would be ideal yes

anmonteiro16:09:41

it’s what I’m starting to realize, at least 🙂

anmonteiro18:09:41

@dnolen am I correct in assuming that we should emit vars as cljs.user.foo = 42 or should it be cljs.user$$gen_ns$$etc?

dnolen18:09:18

@anmonteiro hrm … I hadn’t really thought about the fact that we can change vars from any ns - Closure probably won’t care

dnolen18:09:34

so maybe we don’t need the generated name for anything other than to make goog.require work?

anmonteiro18:09:49

@dnolen I think that’s what I have currently

dnolen18:09:54

right way simpler

anmonteiro19:09:00

but there’s 1 problem

anmonteiro19:09:44

goog.provide('cljs.user$$gen_ns$$_asd1084541096');
goog.require('cljs.core');
goog.require('clojure.set');
cljs.user.x = clojure.set.union.call(null,new cljs.core.PersistentHashSet(null, new cljs.core.PersistentArrayMap(null, 1, [(1),null], null), null),new cljs.core.PersistentHashSet(null, new cljs.core.PersistentArrayMap(null, 1, [(3),null], null), null));

anmonteiro19:09:51

this works no problem

anmonteiro19:09:12

and in the REPL I can see (ns-interns ‘cljs.user => {x ‘cljs.user}

anmonteiro19:09:31

but whenever I evaluate x it returns nil ¯\(ツ)

anmonteiro19:09:47

@dnolen I feel I’m missing something simple

anmonteiro19:09:56

but can't pinpoint exactly what

dnolen19:09:44

what happens when you eval js/cljs.user.x?

dnolen19:09:08

related what happens when you eval (fn [] x)

dnolen19:09:17

(look at the generated source)

anmonteiro19:09:12

in the REPL?

anmonteiro19:09:16

cljs.user=> js/cljs.user.x
nil
cljs.user=> (fn [] x)
#object[ret__5155__auto__ "function (){
return cljs.user.x;
}"]

dnolen19:09:06

@anmonteiro what about js/cljs.user?

anmonteiro19:09:13

empty object

dnolen19:09:40

so it seems like that ns was never loaded

dnolen19:09:10

like that code for the ns was never evaluated

anmonteiro19:09:37

that makes sense

anmonteiro19:09:05

probably because I never goog.require it anywhere 🙂

anmonteiro19:09:35

so where should I do it?

dnolen19:09:10

@anmonteiro I need more context

dnolen19:09:20

what are you doing? compiling, requiring at a REPL?

anmonteiro19:09:57

I’m compiling a src dir which has a file without NS

anmonteiro19:09:13

running the node REPL and expecting x to return what I defined

dnolen19:09:17

right there’s no way to require this thing 🙂

anmonteiro19:09:52

what do you mean?

anmonteiro19:09:57

I’m assuming I have wrong expectations

dnolen19:09:45

doesn’t load-file work?

dnolen19:09:52

from a REPL

dnolen19:09:04

ok cool 🙂

anmonteiro19:09:06

cljs.user=> (load-file "out/cljs/user$$gen_ns$$_asd1084541096.cljs")
WARNING: x at line 3 is being replaced at line 3 out/cljs/user$$gen_ns$$_asd1084541096.cljs
nil
cljs.user=> x
#{1 3}

anmonteiro19:09:21

#{1 3} is what I expected to see, great

anmonteiro19:09:34

so now shouldn’t these files be required automatically ?

dnolen19:09:04

I don’t know what you mean

anmonteiro19:09:58

@dnolen I expected that I could see the definition of x when I entered the REPL

anmonteiro19:09:14

because it’s supposed to be transparent that it’s the cljs.user namespace

dnolen19:09:29

normally you would just (load-file "src/cljs/foo.cljs") or (load “foo.cljs”)

dnolen19:09:38

@anmonteiro no you need to load explicitly

dnolen19:09:41

just like Clojure

anmonteiro19:09:58

OK wait, does this mean it’s working?

anmonteiro19:09:15

so no need for a custom resolver at all 😛

anmonteiro19:09:25

since we can side-effect the cljs.user Closure NS

dnolen19:09:32

that’s right so this will probably be super simple

anmonteiro19:09:56

what about the undeclared var warning when loading the file?

dnolen19:09:03

something is up there

anmonteiro19:09:03

or rather, replace

dnolen19:09:28

probably just some bad logic in the replace check

anmonteiro19:09:48

@dnolen should I be seeing this upon entering the REPL?

cljs.user=> (ns-interns 'cljs.user)
{x #'cljs.user/x}

anmonteiro19:09:52

without loading the file

anmonteiro19:09:57

maybe that’s what’s up

dnolen19:09:09

there’s no implicit loading

anmonteiro19:09:10

well the problem is that the analyzer side-effects the compiler env upon analyzing the file

anmonteiro19:09:39

so x is in there in the NS’s :defs

dnolen19:09:51

so the problem is this (load-file "out/cljs/user$$gen_ns$$_asd1084541096.cljs”)

dnolen19:09:54

that’s not right

dnolen19:09:13

(load-file “src/foo.cljs”)

dnolen19:09:29

if you load that other thing then the compiler will be correctly confused

anmonteiro19:09:40

still happening

dnolen19:09:48

(also this means we don’t need to warn about replacing stuff in cljs.user already done)

anmonteiro19:09:49

cljs.user=> (load-file "src/asd.cljs")
WARNING: x at line 3 is being replaced at line 3 src/asd.cljs
nil

dnolen19:09:15

so then I don’t know, will need to poke around

dnolen19:09:19

but it doesn’t make sense to me

anmonteiro19:09:38

well you haven’t seen my implementation so I’m most definitely messing something up

dnolen19:09:57

sure put a diff somewhere, might be simple

dnolen19:09:01

@anmonteiro the problem is parse-ns

dnolen19:09:15

you need to exit immediately if it’s not one of the special forms

dnolen19:09:26

other wise you’re going to analyze defs

anmonteiro19:09:30

makes total sense

anmonteiro19:09:49

now that you say it 🙂

dnolen19:09:01

@anmonteiro btw for the hash part what we really want is the first few characters of the content-sha of the URI of the file, there’s an example of this in closure.clj (just pointing this out when you have time to get to it)

anmonteiro19:09:47

@dnolen yeah it’s all very WIP still, but I didn’t know that. I’ll do that, thanks

anmonteiro19:09:02

should be this, right?

dnolen19:09:27

@anmonteiro so it looks like this will just be very simple and remove my REPL hacks 😛

anmonteiro19:09:44

@dnolen yes, completely unexpected 🙂

anmonteiro19:09:54

probably still missing the part about :toplevel checks

dnolen19:09:08

right but that's just validation stuff

anmonteiro19:09:17

yeah… I just wanted to prototype something quickly

anmonteiro19:09:27

and it kinda worked, I’m amazed

dnolen19:09:56

this will be a very nice enhancement - very old request

anmonteiro19:09:27

@dnolen removing the REPL hacks will probably need to be a different ticket

dnolen19:09:42

yes that’s fine

dnolen19:09:53

it’s probably best since that might introduce churn anyway

anmonteiro19:09:13

when are you thinking about cutting a CLJS release?

dnolen19:09:42

your self-require self-host thing, and Closure bump are high priority for the next release

dnolen19:09:50

but I’d also like to get this one in here as well

dnolen19:09:58

and I need to port Rich’s spec perf stuff

dnolen19:09:21

so there’s no rush

anmonteiro19:09:22

well I have a little time to work on this in the next few days, so I should have something ready

dnolen19:09:31

1.9.229 seems to be solid for most people

anmonteiro19:09:02

there was a little churn with :rename & spec stuff

anmonteiro19:09:12

but 1.9.229 seems stable enough

mfikes20:09:23

Chelimsky has a successful fix in the works (in transit-java) that would fix http://dev.clojure.org/jira/browse/CLJS-1761 in the future

darwin20:09:26

ad the async repl bootstrap conversation from #clojurescript channel, this is my proposal (not tested yet): https://github.com/darwin/clojurescript/commit/b7bc7377ca6ea2671629d561df741e0436fc03c9 ^ @bendlas @dnolen - comments welcome, I will craft a JIRA ticket with a proper patch if this looked good to you have to run now

dnolen20:09:00

@darwin that patch is a bit too complicated for me - first need to understand what you are trying to accomplish

dnolen20:09:03

then can grok implementation

dnolen20:09:22

I’ll be doing a bunch of CLJS tomorrow so happy to talk through it then

anmonteiro20:09:10

@dnolen hrm still getting the var replaced warning after fixing parse-ns

dnolen20:09:12

so we need to form a hypothesis, what would analyze that code to side effect the environment such that this would happen?

anmonteiro20:09:32

@dnolen I think emit* would

anmonteiro20:09:38

in cljs.compiler

dnolen20:09:52

how? emit* doesn’t analyze does it?

anmonteiro20:09:05

wait maybe I got the wrong func

anmonteiro20:09:18

oh no, it does, actually

anmonteiro20:09:43

here in my diff^

anmonteiro20:09:11

or maybe I made it analyze everything, let me check

anmonteiro20:09:35

oh it’s not emit*, I mean emit-source

anmonteiro20:09:47

always have trouble when the diff is not fully expanded

dnolen20:09:59

@anmonteiro so that logic in emit :ns just seems wrong to me

dnolen20:09:15

my first question is how do you even get here given there is no ns form?

anmonteiro20:09:49

@dnolen it’s emit-source

anmonteiro20:09:04

the diff is misleading when not expanded

anmonteiro20:09:05

@dnolen sorry I might have been running a stale version of the compiler

anmonteiro20:09:22

definitely human error

anmonteiro20:09:32

parse-ns was definitely the culprit

dnolen20:09:34

@anmonteiro ok, yeah it seemed improbable

tomjkidd21:09:30

I tried to follow the instructions at [Building the compiler](https://github.com/clojure/clojurescript/wiki/Building-the-compiler) for windows, and I am getting the following error

tomjkidd21:09:07

Exception in thread "main" java.io.FileNotFoundException: Could not locate cljs/closure__init.class or cljs/closure.clj on classpath., compiling:(C:\Users\tkidd\github\clojurescript\script\aot.clj:1:1)

tomjkidd21:09:35

I looked at the script and see that ./script_aot is trying to run a 'java -server' command

tomjkidd21:09:21

:src/main/clojure:src/main/cljs is at the end of the classpath, so it should be able to find cljs/closure.clj, but it doesn't

tomjkidd21:09:14

I know this is probably specific to my setup, I am using Git Bash to try to do it, but just figured I'd ask in case anyone knew

anmonteiro21:09:20

@tomjkidd might as well be a windows-specific thing. AFAIK there aren’t many people hacking on the compiler using Windows

tomjkidd21:09:04

Yeah, I have an OSX rig which I use mostly, but wanted to try it out.

anmonteiro21:09:18

can you build it in OSX?

tomjkidd21:09:28

I'll go give that a quick try

anmonteiro21:09:41

FWIW current master builds correctly, I’ve been doing it today 🙂

anmonteiro21:09:25

@tomjkidd also if you find out it’s a windows-specific thing, I think I remember @dnolen saying he would take Windows patches easily!

tomjkidd21:09:57

I'll see what I can find

tomjkidd21:09:33

Yeah, building presumes that maven is installed

tomjkidd21:09:40

I don't have that on OSX

tomjkidd21:09:52

I'll install it and try again

tomjkidd22:09:57

@anmonteiro works fine on my mac, like you expected.

anmonteiro22:09:32

@tomjkidd so bad news is you have to figure out what’s stopping it from working on Windows

anmonteiro22:09:45

I don’t have a windows machine at hand to help you, sorry

tomjkidd22:09:22

No worries. I'll keep plugging at it, won't spend too much more time if it's not going anywhere.

anmonteiro22:09:00

@tomjkidd well actually, don’t the paths in windows have different slashes or something?

anmonteiro22:09:10

might as well be the classpath that’s being printed out wrong

anmonteiro22:09:27

e.g. src/main/clojure/ when it should be src\main\clojure

dnolen22:09:40

@tomjkidd the issue is simple as @anmonteiro notes, that script isn’t cross platform

dnolen22:09:00

note that we have a couple of powershell scripts for Windows there, but not one for build

tomjkidd22:09:40

yeah, I was hoping that using Git Bash (which uses MINGW64) I could pull it off. It got pretty far, I've been pushing it along as I encounter new failures

dnolen22:09:50

@tomjkidd that would probably almost work, but I suspect it falls down when giving the classpath to Java

dnolen22:09:58

Git Bash isn’t going to help there

tomjkidd22:09:01

yeah, that's where I'm at

tomjkidd22:09:00

Adjusting the classpath to be in the form /c/Users/tkidd... instead of C:\Users\tkidd does get the 'java server' command to run without failing

tomjkidd22:09:50

I was able to use a clojure repl to transform the command, and that's definitely it.

tomjkidd22:09:35

I noticed the maven plugin dependency:build-classpath is used to create this, and it has some options, but I don't changing it's pathSeparator or fileSeparator will do the right thing here

tomjkidd22:09:57

*but I don't think changing...

tomjkidd23:09:27

Changing line 7 to mvn -f pom.template.xml dependency:build-classpath -Dmdep.outputFile=$CP_FILE -Dmdep.prefix='' -Dmdep.fileSeparator='//'

tomjkidd23:09:11

and line 15 to java -server -cp "$CLJS_CP;src/main/clojure;src/main/cljs" clojure.main script/aot.clj

tomjkidd23:09:19

and I can build on windows

tomjkidd23:09:12

Sorry, line 7 should have been mvn -f pom.template.xml dependency:build-classpath -Dmdep.outputFile=$CP_FILE -Dmdep.fileSeparator='//' -Dmdep.pathSeparator=';'

tomjkidd23:09:46

So it is the way that file/path separators are specified