Fork me on GitHub
#cljs-dev
<
2018-02-18
>
dnolen01:02:51

@bronsa any chance we could get a release just with that fix? I’m now seeing this whenever I start a ClojureScript REPL

dnolen02:02:17

@mfikes re default, are you thinking that we should default to the browser case? I’m fine that

mfikes02:02:15

My actual motivation was for building iOS apps, where plain output is needed, not targeting or assuming any particular environment. (This could also be said of the code compiled for Planck). Since those use cases are rare, perhaps this is an argument for the ability to indicate that you don't really want a target, but with that ability not being the default behavior.

dnolen02:02:05

so -t default doesn’t work?

mfikes02:02:15

Oh, I was unaware of that. Let me try.

mfikes02:02:52

clojure -m cljs.main -t default -o out/main.js -c foo.core
produces an out/main.js that contains browser-specific constructs (`document.write` calls, for example)

dnolen02:02:54

hrm I guess we don’t have what you want though? There’s never been a “default” target, since you at least have to be able to load files

dnolen02:02:03

or are you asking that just need something for Ambly?

mfikes02:02:30

Yes, it is fair that this covers the needs for something like Ambly, which makes it less important.

mfikes02:02:52

And for Ambly

clojure -e "(require 'cljs.build.api)" -e '(cljs.build.api/build "src" {:output-to "out/main.js"})'
produces nice "targetless" code.

dnolen02:02:15

I guess what Ambly really needs is for the target bit to be open

dnolen02:02:28

so you can supply your own thing

mfikes02:02:43

So, I asked myself: "How can I use -c to produce the same thing that you get using that clojure line above?"

dnolen02:02:11

@mfikes hrm what’s in out/main.js?

mfikes02:02:30

Let me paste both for comparison... (they are both smallish)

mfikes02:02:11

clojure -e "(require 'cljs.build.api)" -e '(cljs.build.api/build "src" {:output-to "out/main.js"})' generates

goog.addDependency("base.js", ['goog'], []);
goog.addDependency("../cljs/core.js", ['cljs.core'], ['goog.string', 'goog.Uri', 'goog.object', 'goog.math.Integer', 'goog.string.StringBuffer', 'goog.array', 'goog.math.Long']);
goog.addDependency("../process/env.js", ['process.env'], ['cljs.core']);
goog.addDependency("../foo/core.js", ['foo.core'], ['cljs.core']);

mfikes02:02:08

clojure -m cljs.main -o out/main.js -c foo.core generates

if(typeof Math.imul == "undefined" || (Math.imul(0xffffffff,5) == 0)) {
    Math.imul = function (a, b) {
        var ah  = (a >>> 16) & 0xffff;
        var al = a & 0xffff;
        var bh  = (b >>> 16) & 0xffff;
        var bl = b & 0xffff;
        // the shift by 0 fixes the sign on the high part
        // the final |0 converts the unsigned value into a signed value
        return ((al * bl) + (((ah * bl + al * bh) << 16) >>> 0)|0);
    }
}

var CLJS_OUTPUT_DIR = "out";
load((new java.io.File(new java.io.File("out","goog"), "base.js")).getPath());
load((new java.io.File(new java.io.File("out","goog"), "deps.js")).getPath());
load((new java.io.File(new java.io.File(new java.io.File("out","goog"),"bootstrap"),"nashorn.js")).getPath());
load((new java.io.File("out","cljs_deps.js")).getPath());
goog.global.CLOSURE_UNCOMPILED_DEFINES = {"cljs.core._STAR_target_STAR_":"nashorn"};
goog.require("process.env");
goog.require("foo.core");

mfikes02:02:34

Adding -t default causes the above to change to:

var CLOSURE_UNCOMPILED_DEFINES = {"cljs.core._STAR_target_STAR_":"default"};
var CLOSURE_NO_DEPS = true;
if(typeof goog == "undefined") document.write('<script src="out/goog/base.js"></script>');
document.write('<script src="out/goog/deps.js"></script>');
document.write('<script src="out/cljs_deps.js"></script>');
document.write('<script>if (typeof goog == "undefined") console.warn("ClojureScript could not load :main, did you forget to specify :asset-path?");</script>');
document.write('<script>goog.require("process.env");</script>');
document.write('<script>goog.require("foo.core");</script>');

mfikes02:02:05

FWIW for Planck, I don't specify :target in the compiler opts map passed to cljs.build.api/build and then Planck loads the resulting main.js that consists of nothing but clean goog.addDependency calls. So perhaps this capability is only really useful for environments that take on the responsibility for bootstrapping the Closure environment themselves. This is a nice capability from a "flexibility" standpoint, but clearly not what most end-users will be taking advantage of.

mfikes02:02:49

To be honest, directly calling cljs.build.api/build is fine for this, and https://dev.clojure.org/jira/browse/CLJS-2532 could be declined until we hear from other users out there that there is truly a need for a clean "targetless" compile capability via cljs.main and -c.

mfikes02:02:46

Interestingly, if you look at the very first out/main.js generated on the Quick Start page, it is of this "targetless" variety, so if we tried to replace all of the commands on that page with updated ones using cljs.main and variations involving -c and -r we would perhaps run into the same problem there. (Or we could just go with revisions that immediately start using -t browser or somesuch.)

mfikes02:02:37

Ahh, I see what I'm doing... by specifying a namespace after -c this is the same as adding a :main compiler option, which causes the different out/main.js to be written.

dnolen03:02:18

ah right, so yeah maybe special casing :target :none is the way to go

mfikes03:02:33

Perhaps yeah. What is more important than figuring out anything for Planck or Ambly, is what the first build or two in Quick Start looks like and whether it uses cljs.build.api/build without :main or cljs.main with -c and a :target :none or a -c without a namespace or something else...

dnolen03:02:27

@mfikes well the tutorial is focused on doing stuff in browser / node - so I think we can just stick with that

dnolen03:02:42

I don’t see a need to cover that there - unless I missed something

mfikes03:02:13

Yeah, the very first build could end up looking like clojure -m cljs.main -o "out/main.js" -c hello-world.core, essentially jumping initially into the Less Boilerplate section.

dnolen03:02:39

@mfikes just pushed :none case, we just skip the main bits if you set -t none

mfikes03:02:04

Nice. I confirmed that clojure -m cljs.main -t none -o out/main.js -c foo.core essentially provides what I asked for in https://dev.clojure.org/jira/browse/CLJS-2532

mfikes13:02:27

@dnolen I was thinking about digging into :analyze-path https://dev.clojure.org/jira/browse/CLJS-2535 but if you already have an idea on that one and were thinking of working on it, let me know. (Don't want to duplicate effort.)

dnolen13:02:13

@mfikes not working on that, go for it!

dnolen14:02:46

@juhoteperi hrm I get test failure now for a npm modules test when running ./script/test

dnolen14:02:15

seems odd since haven’t been working on anything related to that, the error I see over here is that using lodash nth got renamed to ii somehow, but the original fn name wasn’t changed

mfikes15:02:05

@dnolen Did you upgrade your dev box to Java 9? If so, perhaps you are now seeing https://dev.clojure.org/jira/browse/CLJS-2400

mfikes15:02:09

FWIW, I avoid this locally by having done brew cask install java8 to also have Java 8 on my box, and by additionally

export JAVA_HOME=`/usr/libexec/java_home -v 1.8`

dnolen15:02:47

@mfikes oh hrm so is a JDK 9 bug in Closure or something?

mfikes15:02:48

(You can easily switch to Java 9 by changing that to -v 9.0

mfikes15:02:15

Yeah, I wonder if we can form a minimal repro or if you need everything at play. Someone else encountered this in #clojurescript the other day https://clojurians.slack.com/archives/C03S1L9DN/p1518811207000651

dnolen15:02:55

yeah that’s probably it

dnolen16:02:35

yeah can verify that issue on JDK 9

mfikes16:02:01

Wow, looks like a bit of a mess.

mfikes16:02:01

(Apache Ignite appears to work around it by delving into JDK internals.)

dnolen16:02:37

@bronsa still more :refer issues with 1.2.2, you :refer :all your impl stuff in clojure.tools.reader but you are missing :excludes

bronsa16:02:57

that's intentional

bronsa16:02:32

it will only refer tagged-literal/`reader-conditional` & co if loading <=clojure-1-7-alpha5

bronsa16:02:37

otherwise they won't be defined

bronsa16:02:09

those forms are conditionally evaluated

bronsa16:02:11

which now that i think about it means the exclude stuff wasn't really necessary

bronsa16:02:19

so curious about how you're seeing this

dnolen16:02:28

oh hrm, now it makes sense, I’m using the AOTed artifacts, but they will have been compiled by some version of Clojure already

bronsa16:02:58

that will be an issue

dnolen16:02:43

and I guess you can’t :exclude something that doesn’t exist?

bronsa16:02:25

I can but then it wouldn't work as monkey patch for <=1.7-alpha5

dnolen16:02:10

but I don’t really understand why you need to backport those defs into older version of Clojure?

bronsa16:02:19

I had no idea we were distributing aot

bronsa16:02:42

@dnolen because I have to conditionally use the c.c or the c.t.r.utils versions depending on which is available

dnolen16:02:11

ClojureScript has been AOTed for some time now - and I had Alex modify the tools.reader and data.json deps so I could grab those as well

dnolen16:02:21

I didn’t include them before directly because wanted to wait an ssee

dnolen16:02:28

but I’d never seen any problems before until now

bronsa16:02:31

doesn't cljs work only with > 1.7 anyway?

bronsa16:02:46

why are we compiling it with an older version

dnolen16:02:47

yes, I’m pretty sure rely on 1.8 features

dnolen16:02:05

oh so I guess that’s the problem?

dnolen16:02:09

I should compile it with 1.9?

bronsa16:02:24

then those monkey patch functions won't ever get compiled

dnolen16:02:31

@bronsa but wait I guess I don’t see why this should be a problem since I’ve been compiling with 1.8?

bronsa16:02:10

yeah that shouldnt happen

bronsa16:02:09

i dont see how that's possible

bronsa16:02:02

can you double check something's not causing an older version of clojure to be used

bronsa16:02:11

i'll do some experimentation if you're sure you're using 1.8

dnolen16:02:32

@bronsa well I see that excluding names that don’t exist is not really a problem

dnolen16:02:49

so I don’t see how that affects the monkey-patching bit you’re talking about

dnolen16:02:25

@mfikes hrm the Java9Bridge thing there is pretty simple if yuck-y - I’m not sure we’re going to come up with better ideas

bronsa16:02:48

on <1.7, c.c doesn't have those vars defined, so what we need is effectively (require [.. util :refer [tagged-literal]]) so that in code when we use tagged-literal that's the var we'll be refering too

bronsa16:02:57

tools.reader uses tagged-literal in its code

bronsa16:02:37

altho I wonder if i'm not using the wrong versionn for one of tagged-literal or reader-conditional

bronsa16:02:38

let me check

dnolen16:02:56

k, also maybe you can just do this choice a bit more dynamically, always exclude, resolve then choose or something like that

bronsa16:02:06

doing :refer :all w/o exclude is a way to refer to tagged-literal when it doesn't exist in c.c, but not when it does

dnolen16:02:15

fwiw, feature detection seems more reliable for stuff like this

bronsa16:02:17

yes if this is a problem I could exclude by default and conditionally require

dnolen16:02:43

(instead of version detection)

bronsa16:02:44

@dnolen no good way to do this otherwise since we need to define types not just vars

dnolen16:02:26

ah k, surface is bigger than I thought poking around

bronsa16:02:17

yeah, I might drop support for clojure < 1.7.0 at some point but until then there's no other way to do this

bronsa16:02:49

other alternatives don't play well with reloading bc of classloaders

dnolen16:02:57

k going to leave this alone for now then - the load time benefit from AOTed tools.reader and data.json isn’t very big - that’s why I was bothering

bronsa16:02:00

or would prevent AOT at all

mfikes16:02:30

Yeah, @dnolen we could port the relevant portions to Clojure. I could take a stab at it if you are not already.

thheller16:02:42

there is always (System/getProperty "java.class.path") which is most likely good enough

mfikes17:02:15

To @thheller’s point, it looks like we currently get 26 items out of all-classpath-urls, but only 15 out of (System/getProperty "java.class.path") so perhaps a port is the right approach

rauh18:02:12

There is also (:classPath (bean (ManagementFactory/getRuntimeMXBean))). Not sure if that's any help. Didn't read all messages here.

rauh18:02:13

And (:bootClassPath (bean (ManagementFactory/getRuntimeMXBean)))

rauh18:02:14

Throw in Java9 and can be checked with (:bootClassPathSupported (bean (ManagementFactory/getRuntimeMXBean)))

thheller17:02:44

yeah but the extra jars are jvm internals. you won't find .cljs in there so you don't need them

dnolen17:02:14

one thing that’s a little frustrating is that the CLJS uberjar is 3 seconds faster to do anything, and this appears to be due to everything in the uberjar being compiled and direct linked

dnolen17:02:56

@thheller I think what @mfikes is suggesting is probably the safest in terms of emulating precisely how it worked before

dnolen17:02:13

my experience is that the tiniest differences with stuff like this inevitable is a problem for someone

thheller17:02:10

the only difference is that the classloader path will pick up custom classloaders

dnolen17:02:29

and people use those and put stuff where those things will find them 🙂

thheller17:02:30

it might affect boot yes

mfikes17:02:11

I'll put together a straight port... it looks like very little code, and we can decide whether to use it.

thheller17:02:38

should probably try/catch and fall back to the env property. the Java code doesn't look all that trustworthy and looks like it expects that you pass in an URLClassLoader ... which custom classloaders might not be.

dnolen17:02:46

for the time being the important is to just emulate the old behavior such that it works under Java 9 - we can enhance etc. some other time

dnolen17:02:58

I would just like Java 9 to work like Java 8 - nothing more

dnolen17:02:36

(talking about priorities for the next release)

mfikes18:02:25

@dnolen Bah! Oracle have secured access to that stuff. Specifically if you try to

(-> (Class/forName "jdk.internal.loader.BuiltinClassLoader") (.getDeclaredField "ucp") (.setAccessible true))
things will fail with
InaccessibleObjectException Unable to make field private final jdk.internal.loader.URLClassPath jdk.internal.loader.BuiltinClassLoader.ucp accessible: module java.base does not "opens jdk.internal.loader" to unnamed module @4c03a37  java.lang.reflect.AccessibleObject.checkCanSetAccessible (AccessibleObject.java:337)
unless you add something like --add-opens java.base/jdk.internal.loader=ALL-UNNAMED to your Java options when you launch your VM.

dnolen18:02:03

@mfikes, hrm I guess in Java 9 crawling the classpath like this is just not really supported, I’m fine with going with @thheller’s suggestion

dnolen18:02:15

anybody that was expecting more will have their own issues now anyway

dnolen18:02:37

but I think this will probably slow Java 9 adoption

dnolen18:02:57

so we should support the old way - only look at classpath if URLClassloader isn’t present

mfikes18:02:58

Yeah, I found a couple other codebases out there messing with that "ucp" field, but interestingly I don't see commits that add --add-opens java.base/jdk.internal.loader=ALL-UNNAMED in those projects.

dnolen18:02:44

I just bumped ClojureScript version to 1.10, I’m going to do pREPL for the next release

mfikes18:02:53

Yeah, I can make it try to work based on (System/getProperty "java.class.path") if Java 9.

thheller18:02:14

FWIW thats what I have in shadow-cljs. if the classloader returns items I use that, it not I use the fallback. https://github.com/thheller/shadow-cljs/blob/master/src/main/shadow/build/classpath.clj#L505-L513

mfikes18:02:14

Unit tests pass with that change 🙂

rauh18:02:59

You can also check if it's supported:

(select-keys (bean (ManagementFactory/getRuntimeMXBean))
               [:bootClassPathSupported
                :bootClassPath
                :classPath])

richiardiandrea18:02:37

@dnolen I know it is not a priority, but I was wondering if you could check CLJS-2492

dnolen18:02:59

@richiardiandrea I don’t understand the patch (but also maybe because the old thing had problems)

dnolen18:02:13

if there’s no name you use still used the munged name?

dnolen18:02:43

that you use (symbol mn) even if you don’t have a name seems strange to me

dnolen18:02:47

(it was strange before too)

dnolen18:02:33

we would throw away stuff when name is nil it seems

richiardiandrea18:02:59

Uhm, that is something I haven't considered actually (the nil case)

dnolen18:02:13

well might as well fix that up if you’re there

richiardiandrea18:02:18

Yes definitely, I will handle that case, I see that if there is no name a cljs-timestamp is used

richiardiandrea18:02:59

I also want to mention that name is munged and used as source map key somewhere

richiardiandrea18:02:08

Will ping you again when the new version is ready, thanks for checking

richiardiandrea19:02:46

I have to say that having the test alias in deps.edn is a great win, and on top of that I added the rebel global alias for Bruce super powers 😃 Good job overall.

richiardiandrea21:02:21

@dnolen as promised, ping for CLJS-2492, also added one more test

dnolen22:02:06

@mfikes patch fixed the Java 9 issue for me

mfikes22:02:31

Cool. I suspect ClojureScript 1.10 will be good to go with Java 9, with the only corner case being Nashorn not being able to run self-host, which is arguably not that big of a deal. https://dev.clojure.org/jira/browse/CLJS-2530