ClojureScript

nodejs target no longer works

Details

  • Type: Defect Defect
  • Status: Resolved Resolved
  • Priority: Major Major
  • Resolution: Completed
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: None
  • Labels:
    None
  • Environment:
    OS X

Description

If you target nodejs by adding {:target :nodejs} to the options (and set main-cli-fn) , you get this error:

https://gist.github.com/3892983

This appears to have been introduced with this commit: https://github.com/clojure/clojurescript/commit/d6f7d0b193de22378e06298fa543ca57d570c001

It spits out that error and nothing else. Any versions before that works fine.

Related: CLJS-355

Attachments

  1. File
    cljs-395-nodetarget.diff
    20/Oct/12 11:09 AM
    1 kB
    Paul deGrandis
  2. File
    cljs-395-nodetarget-2.diff
    20/Oct/12 11:33 AM
    2 kB
    Paul deGrandis
  3. File
    cljs-395-nodetarget-final.diff
    23/Oct/12 10:50 AM
    1 kB
    Paul deGrandis
  4. Text File
    work-around-cljs-395.patch
    17/Oct/12 12:50 AM
    0.9 kB
    Tom Jack

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Tom Jack added a comment - - edited

A workaround that doesn't need a patch is to just require cljs.nodejs somewhere in your project.

Here is a workaround patch :/ (work-around-cljs-395.patch, 16 Oct 2012). The patch just removes the dependency on cljs.nodejs from cljs.nodejscli. Since all it uses is (js* "process"), it didn't seem worth the work to figure out how to do the dependency resolution while ensuring nodejscli ends up at the bottom.

Show
Tom Jack added a comment - - edited A workaround that doesn't need a patch is to just require cljs.nodejs somewhere in your project. Here is a workaround patch :/ (work-around-cljs-395.patch, 16 Oct 2012). The patch just removes the dependency on cljs.nodejs from cljs.nodejscli. Since all it uses is (js* "process"), it didn't seem worth the work to figure out how to do the dependency resolution while ensuring nodejscli ends up at the bottom.
Hide
Permalink
David Nolen added a comment -

We'll have to ask Paul degGrandis (ohpauleez on IRC) about this one.

Show
David Nolen added a comment - We'll have to ask Paul degGrandis (ohpauleez on IRC) about this one.
Hide
Permalink
Paul deGrandis added a comment -

I can't recreate this on master or on my personal branch (where I maintain my own set of patches and changes).

Could someone link to a git repo that has code that can produce this for me?
Thanks!

Show
Paul deGrandis added a comment - I can't recreate this on master or on my personal branch (where I maintain my own set of patches and changes). Could someone link to a git repo that has code that can produce this for me? Thanks!
Hide
Permalink
James Long added a comment -

That's surprising. Does Tom's comment help at all?

This deterministically reproduces it for me. I forked the clojurescript repo today and added a test file:

git clone git@github.com:jlongster/clojurescript.git && \
 cd clojurescript && \
 ./script/bootstrap && \
 ./bin/cljsc hello.cljs '{:optimizations :advanced :target :nodejs}' > hello-output.js
Show
James Long added a comment - That's surprising. Does Tom's comment help at all? This deterministically reproduces it for me. I forked the clojurescript repo today and added a test file: 
git clone git@github.com:jlongster/clojurescript.git && \
 cd clojurescript && \
 ./script/bootstrap && \
 ./bin/cljsc hello.cljs '{:optimizations :advanced :target :nodejs}' > hello-output.js
Hide
Permalink
Paul deGrandis added a comment -

James, I'll take a look at that and start tracing things through. I'll have this all patched up ASAP.

I'm actually in the middle of working on two Node.js CLJS apps. I'd really like to see CLJS's system scripting story come alive

Show
Paul deGrandis added a comment - James, I'll take a look at that and start tracing things through. I'll have this all patched up ASAP. I'm actually in the middle of working on two Node.js CLJS apps. I'd really like to see CLJS's system scripting story come alive
Hide
Permalink
James Long added a comment -

Awesome, thanks! I agree, I'd love to use CLJS to interact with node stuff.

Show
James Long added a comment - Awesome, thanks! I agree, I'd love to use CLJS to interact with node stuff.
Hide
Permalink
Paul deGrandis added a comment - - edited

That still doesn't recreate it for me: http://www.pauldee.org/StillNoBug.png

I'm running:
Java 1.7.0_04 Java HotSpot(TM) 64-Bit Server VM
On:
OSX 10.7.4 (4-core i7)
With:
Node v0.8.8

=======

I get the same result with and without CLOJURESCRIPT_HOME set.

Show
Paul deGrandis added a comment - - edited That still doesn't recreate it for me: http://www.pauldee.org/StillNoBug.png I'm running: Java 1.7.0_04 Java HotSpot(TM) 64-Bit Server VM On: OSX 10.7.4 (4-core i7) With: Node v0.8.8 ======= I get the same result with and without CLOJURESCRIPT_HOME set.
Hide
Permalink
James Long added a comment -

Wow, that is weird. I guess you can close this bug if nobody else can reproduce it. It'd be great if at least one other person could try on a different computer.

Otherwise, maybe I have some weird environment var set that's messing up things. I'll come back to it at some point and see if I can dig into it more. Until then, feel free to close this bug.

Show
James Long added a comment - Wow, that is weird. I guess you can close this bug if nobody else can reproduce it. It'd be great if at least one other person could try on a different computer. Otherwise, maybe I have some weird environment var set that's messing up things. I'll come back to it at some point and see if I can dig into it more. Until then, feel free to close this bug.
Hide
Permalink
Paul deGrandis added a comment -

I'm going to leave it open and reduce it to minor. I want more environments to try it out - it definitely seems like something buried deep within

Show
Paul deGrandis added a comment - I'm going to leave it open and reduce it to minor. I want more environments to try it out - it definitely seems like something buried deep within
Hide
Permalink
Tom Jack added a comment - - edited

I can't reproduce with hello.cljs either. But I get "No print-fn fn set for evaluation environment". Why don't you?

I can, however, reproduce with mori:

$ GIT_DIR=$CLOJURESCRIPT_HOME/.git git rev-parse HEAD
e3ed0e7b69f9522e8759d0a6567afabb2a98d949
$ git clone https://github.com/swannodette/mori.git && cd mori
...
$ $CLOJURESCRIPT_HOME/bin/cljsc src '{:optimizations :advanced :target :nodejs}' > mori.js
Oct 19, 2012 4:18:13 PM com.google.javascript.jscomp.LoggerErrorManager println
SEVERE: cljs.nodejscli:3: ERROR - required "cljs.nodejs" namespace never provided
goog.require('cljs.nodejs');
^

Oct 19, 2012 4:18:13 PM com.google.javascript.jscomp.LoggerErrorManager printSummary
WARNING: 1 error(s), 0 warning(s)
$ cat mori.js
ERROR: JSC_MISSING_PROVIDE_ERROR. required "cljs.nodejs" namespace never provided at cljs.nodejscli line 3 : 0
#!/usr/bin/env node

My workaround patch fixes it.

EDIT: actually, my workaround patch was applied in CLOJURESCRIPT_HOME when I tried hello.cljs. Without the patch, I get the same error.

Show
Tom Jack added a comment - - edited I can't reproduce with hello.cljs either. But I get "No print-fn fn set for evaluation environment". Why don't you? I can, however, reproduce with mori: 
$ GIT_DIR=$CLOJURESCRIPT_HOME/.git git rev-parse HEAD
e3ed0e7b69f9522e8759d0a6567afabb2a98d949
$ git clone https://github.com/swannodette/mori.git && cd mori
...
$ $CLOJURESCRIPT_HOME/bin/cljsc src '{:optimizations :advanced :target :nodejs}' > mori.js
Oct 19, 2012 4:18:13 PM com.google.javascript.jscomp.LoggerErrorManager println
SEVERE: cljs.nodejscli:3: ERROR - required "cljs.nodejs" namespace never provided
goog.require('cljs.nodejs');
^

Oct 19, 2012 4:18:13 PM com.google.javascript.jscomp.LoggerErrorManager printSummary
WARNING: 1 error(s), 0 warning(s)
$ cat mori.js
ERROR: JSC_MISSING_PROVIDE_ERROR. required "cljs.nodejs" namespace never provided at cljs.nodejscli line 3 : 0
#!/usr/bin/env node
My workaround patch fixes it. EDIT: actually, my workaround patch was applied in CLOJURESCRIPT_HOME when I tried hello.cljs. Without the patch, I get the same error.
Hide
Permalink
Paul deGrandis added a comment - - edited

Can you provide the environment this is happening in?

EDIT:
I'm not a fan of the patch because it isn't a fix, it's just a simple workaround.
Can anyone spot the reason why the nodejs code isn't being shoved into the end of the file on certain platforms?

Seems like the commit cited above should always do that?

Show
Paul deGrandis added a comment - - edited Can you provide the environment this is happening in? EDIT: I'm not a fan of the patch because it isn't a fix, it's just a simple workaround. Can anyone spot the reason why the nodejs code isn't being shoved into the end of the file on certain platforms? Seems like the commit cited above should always do that?
Hide
Permalink
Tom Jack added a comment -

cljs.nodejscli is being shoved into the end, but it is not analyzed for dependencies. So unless you require cljs.nodejs somewhere in your project, it is never pulled in. No clue why it works for you...

My environment:
ubuntu 12.04
java version "1.6.0_30"
Java(TM) SE Runtime Environment (build 1.6.0_30-b12)
Java HotSpot(TM) 64-Bit Server VM (build 20.5-b03, mixed mode)

Show
Tom Jack added a comment - cljs.nodejscli is being shoved into the end, but it is not analyzed for dependencies. So unless you require cljs.nodejs somewhere in your project, it is never pulled in. No clue why it works for you... My environment: ubuntu 12.04 java version "1.6.0_30" Java(TM) SE Runtime Environment (build 1.6.0_30-b12) Java HotSpot(TM) 64-Bit Server VM (build 20.5-b03, mixed mode)
Hide
Permalink
Paul deGrandis added a comment -

Ahh I see the problem - I'll apply an appropriate patch.

It needs to be added to the end, but before the add-dependencies call.

Show
Paul deGrandis added a comment - Ahh I see the problem - I'll apply an appropriate patch. It needs to be added to the end, but before the add-dependencies call.
Hide
Permalink
Tom Jack added a comment -

But wasn't that what we had before, and the problem was that add-dependencies didn't necessarily keep nodejscli at the bottom?

Show
Tom Jack added a comment - But wasn't that what we had before, and the problem was that add-dependencies didn't necessarily keep nodejscli at the bottom?
Hide
Permalink
David Nolen added a comment -

Other people on the mailing list are experiencing this.

Show
David Nolen added a comment - Other people on the mailing list are experiencing this.
Hide
Permalink
Paul deGrandis added a comment - - edited

Can someone experiencing the problem screen the patch I attached: cljs-395-nodetarget.diff

The patch is to hand include the nodejs ns before the nodejscli namespace (essentially what add-dependencies would do). The reason for hand including at the end is to prevent the stomping of main-cli-fn - as was fixed in CLJS-355.

=====
EDIT:

Nvm - that won't work. On projects that DO require nodejs, you'll get a "required twice" error.

SEVERE: cljs.nodejs:1: ERROR - namespace "cljs.nodejs" cannot be provided twice
goog.provide('cljs.nodejs');

New patch to try adding nodejs to the add-dependencies call and tacks on the nodejscli at the end, outside of the call to prevent main-cli-fn from getting stomped. I'll refactor the code - just want to see if this fixes the issue for people first.

Show
Paul deGrandis added a comment - - edited Can someone experiencing the problem screen the patch I attached: cljs-395-nodetarget.diff The patch is to hand include the nodejs ns before the nodejscli namespace (essentially what add-dependencies would do). The reason for hand including at the end is to prevent the stomping of main-cli-fn - as was fixed in CLJS-355. ===== EDIT: Nvm - that won't work. On projects that DO require nodejs, you'll get a "required twice" error. 
SEVERE: cljs.nodejs:1: ERROR - namespace "cljs.nodejs" cannot be provided twice
goog.provide('cljs.nodejs');
New patch to try adding nodejs to the add-dependencies call and tacks on the nodejscli at the end, outside of the call to prevent main-cli-fn from getting stomped. I'll refactor the code - just want to see if this fixes the issue for people first.
Hide
Permalink
Tom Jack added a comment -

Looks like your patch makes mori build correctly, thanks. But since mori doesn't set main-cli-fn, it's still broken. But that seems like a separate issue.

Show
Tom Jack added a comment - Looks like your patch makes mori build correctly, thanks. But since mori doesn't set main-cli-fn, it's still broken. But that seems like a separate issue.
Hide
Permalink
Paul deGrandis added a comment -

David, do you want me to clean up the code or does it look fine to you? Sort of sucks to see the double concat and double when - but I can't think of cleaner refactor that doesn't involve just breaking some of it into the let bindings

Show
Paul deGrandis added a comment - David, do you want me to clean up the code or does it look fine to you? Sort of sucks to see the double concat and double when - but I can't think of cleaner refactor that doesn't involve just breaking some of it into the let bindings
Hide
Permalink
David Nolen added a comment -

I'm mostly interested in something that works at this point. We can clean it up later if that's possible. Would like to get feedback from the original ticket creator as well.

Show
David Nolen added a comment - I'm mostly interested in something that works at this point. We can clean it up later if that's possible. Would like to get feedback from the original ticket creator as well.
Hide
Permalink
James Long added a comment -

The patch does indeed work. Sorry for being distant from this bug, I've been consumed with a project at work. I can't give feedback on the patch because I'm not familiar with the compiler.

james:~/projects/clojurescript(master)% ./bin/cljsc hello.cljs '{:optimizations :advanced :target :nodejs}' > hello-output.js
Oct 23, 2012 9:48:59 AM com.google.javascript.jscomp.LoggerErrorManager println
SEVERE: cljs.nodejscli:3: ERROR - required "cljs.nodejs" namespace never provided
goog.require('cljs.nodejs');
^

Oct 23, 2012 9:48:59 AM com.google.javascript.jscomp.LoggerErrorManager printSummary
WARNING: 1 error(s), 0 warning(s)
james:~/projects/clojurescript(master)% patch -p1 < ~/tmp/cljs-395-nodetarget-2.diff 
patching file src/clj/cljs/closure.clj
patching file src/clj/cljs/closure.clj
james:~/projects/clojurescript(master)% ./bin/cljsc hello.cljs '{:optimizations :advanced :target :nodejs}' > hello-output.js
james:~/projects/clojurescript(master)% node hello-output.js 
hello world
Show
James Long added a comment - The patch does indeed work. Sorry for being distant from this bug, I've been consumed with a project at work. I can't give feedback on the patch because I'm not familiar with the compiler. 
james:~/projects/clojurescript(master)% ./bin/cljsc hello.cljs '{:optimizations :advanced :target :nodejs}' > hello-output.js
Oct 23, 2012 9:48:59 AM com.google.javascript.jscomp.LoggerErrorManager println
SEVERE: cljs.nodejscli:3: ERROR - required "cljs.nodejs" namespace never provided
goog.require('cljs.nodejs');
^

Oct 23, 2012 9:48:59 AM com.google.javascript.jscomp.LoggerErrorManager printSummary
WARNING: 1 error(s), 0 warning(s)
james:~/projects/clojurescript(master)% patch -p1 < ~/tmp/cljs-395-nodetarget-2.diff 
patching file src/clj/cljs/closure.clj
patching file src/clj/cljs/closure.clj
james:~/projects/clojurescript(master)% ./bin/cljsc hello.cljs '{:optimizations :advanced :target :nodejs}' > hello-output.js
james:~/projects/clojurescript(master)% node hello-output.js 
hello world
Hide
Permalink
David Nolen added a comment -

Great, thanks James - Paul, could we get a patch that is a single commit? Thanks.

Show
David Nolen added a comment - Great, thanks James - Paul, could we get a patch that is a single commit? Thanks.
Hide
Permalink
Paul deGrandis added a comment -

np, I'll squash it down right now.

Show
Paul deGrandis added a comment - np, I'll squash it down right now.
Hide
Permalink
Paul deGrandis added a comment -

Let me know how that looks.

Show
Paul deGrandis added a comment - Let me know how that looks.
Hide
Permalink
David Nolen added a comment -

Looks good!

Show
David Nolen added a comment - Looks good!

People

Vote (0)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved:
Atlassian JIRA (v4.4#649-r158309) | Report a problem
Powered by a free Atlassian JIRA open source license for Clojure. Try JIRA - bug tracking software for your team.