New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Windows users cannot run CJSE build task #119

Closed
Bartvds opened this Issue Jul 23, 2013 · 18 comments

Comments

Projects
None yet
6 participants
@Bartvds

Bartvds commented Jul 23, 2013

I rebased my code for #109 and now I cannot run the npm scripts because somebody changed the commands to using bash and some shellscript.

That is not really in the spirit of cross-platform code and unnecessary as there are many node.js based build runners.

I use grunt a lot. It is a bit heavy though, maybe you like jake better. Or if you want to keep close to shellscript style go for shelljs

@Bartvds

This comment has been minimized.

Show comment
Hide comment
@Bartvds

Bartvds Jul 23, 2013

I tried shelljs as it's very close to current but it doesn't support range() so I couldn't convert it 1-to-1.

This needs a real decision by a core member.

Until then I'll spin up a VM, but as said a (modern) node.js application shouldn't have to depend on OS specific tooling.

If disagree on this I could also commit a Vagrant file and cookbook for easy testing on a headless Ubuntu on VirtualBox.

Edit: I have the Vagrant file ready, but it is a bit of a nuclear option imho. If the project embraces node.js and welcomes contributions it shouldn't require a few gigabyte of Linux, Ruby, Vagrant, Chef and VirtualBox :)

Bartvds commented Jul 23, 2013

I tried shelljs as it's very close to current but it doesn't support range() so I couldn't convert it 1-to-1.

This needs a real decision by a core member.

Until then I'll spin up a VM, but as said a (modern) node.js application shouldn't have to depend on OS specific tooling.

If disagree on this I could also commit a Vagrant file and cookbook for easy testing on a headless Ubuntu on VirtualBox.

Edit: I have the Vagrant file ready, but it is a bit of a nuclear option imho. If the project embraces node.js and welcomes contributions it shouldn't require a few gigabyte of Linux, Ruby, Vagrant, Chef and VirtualBox :)

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Jul 23, 2013

Member

@Bartvds I use Git Bash on Windows as my primary dev environment, and it works fine for this. I do want to get a formal build system in place eventually, I'm just not sure what to use. I'll look at your suggestions.

Member

nzakas commented Jul 23, 2013

@Bartvds I use Git Bash on Windows as my primary dev environment, and it works fine for this. I do want to get a formal build system in place eventually, I'm just not sure what to use. I'll look at your suggestions.

@Bartvds

This comment has been minimized.

Show comment
Hide comment
@Bartvds

Bartvds Jul 23, 2013

Weird, I had problems with Git Bash some time earlier, I blamed windows. But I solved them by using a different console wrapper (Conemu), and indeed the scripts works fine now. Thanks for motivating :)

Bartvds commented Jul 23, 2013

Weird, I had problems with Git Bash some time earlier, I blamed windows. But I solved them by using a different console wrapper (Conemu), and indeed the scripts works fine now. Thanks for motivating :)

@iancmyers

This comment has been minimized.

Show comment
Hide comment
@iancmyers

iancmyers Sep 5, 2013

Contributor

Can this issue be closed or is this still a problem?

Contributor

iancmyers commented Sep 5, 2013

Can this issue be closed or is this still a problem?

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Sep 6, 2013

Member

Before closing, I want to switch over Travis to use Jake and add Jake setup to the docs.

Member

nzakas commented Sep 6, 2013

Before closing, I want to switch over Travis to use Jake and add Jake setup to the docs.

@ilyavolodin

This comment has been minimized.

Show comment
Hide comment
@ilyavolodin

ilyavolodin Sep 6, 2013

Member

Coverage report is still not working with Jake. Or has it been fixed already?

Member

ilyavolodin commented Sep 6, 2013

Coverage report is still not working with Jake. Or has it been fixed already?

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Sep 7, 2013

Member

It's still not working. I filed a bug on Jake, but he couldn't reproduce on a Linux machine (I'm using windows, are you?).

Member

nzakas commented Sep 7, 2013

It's still not working. I filed a bug on Jake, but he couldn't reproduce on a Linux machine (I'm using windows, are you?).

@ilyavolodin

This comment has been minimized.

Show comment
Hide comment
@ilyavolodin

ilyavolodin Sep 7, 2013

Member

I'm on windows too. It doesn't work for me either. I tried playing with it to see if I could find some work around, but I'm not familiar enough with Jake, I'm much more used to Grunt.

Member

ilyavolodin commented Sep 7, 2013

I'm on windows too. It doesn't work for me either. I tried playing with it to see if I could find some work around, but I'm not familiar enough with Jake, I'm much more used to Grunt.

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Sep 8, 2013

Member

I'm wondering if its related more to Git Bash than Jake. I need to try it on a separate VM when I get a second. The thing is, it is definitely still checking code coverage because you will get a non zero exit code if the coverage drops below the threshold...it's just not outputting the report.

Member

nzakas commented Sep 8, 2013

I'm wondering if its related more to Git Bash than Jake. I need to try it on a separate VM when I get a second. The thing is, it is definitely still checking code coverage because you will get a non zero exit code if the coverage drops below the threshold...it's just not outputting the report.

@ilyavolodin

This comment has been minimized.

Show comment
Hide comment
@ilyavolodin

ilyavolodin Sep 8, 2013

Member

Nether Jake nor Istanbul use any bash specific functionality (as far as I can tell), so it should work in git bash as well as regular command prompt. But it doesn't work in either. Didn't know about it still reporting exit code correctly.

Member

ilyavolodin commented Sep 8, 2013

Nether Jake nor Istanbul use any bash specific functionality (as far as I can tell), so it should work in git bash as well as regular command prompt. But it doesn't work in either. Didn't know about it still reporting exit code correctly.

@albertosantini

This comment has been minimized.

Show comment
Hide comment
@albertosantini

albertosantini Nov 27, 2013

Contributor

The scripts changelog, test and lint work correctly in Git Bash (1.8.4) on Windows (7-8/x64).

npm run bundle output:

> eslint@0.1.3 bundle c:\My\Dev\snippets\eslint
> bash scripts/bundle.sh


c:\My\Dev\snippets\eslint\node_modules\commonjs-everywhere\lib\command.js:167
        throw e;
              ^
Error: Core module "fs" has not yet been ported to the browser
    at resolvePath (c:\My\Dev\snippets\eslint\node_modules\commonjs-everywhere\lib\relative-resolve.js:29:15)
    at module.exports (c:\My\Dev\snippets\eslint\node_modules\commonjs-everywhere\lib\relative-resolve.js:61:16)
    at Controller.estraverse.replace.enter (c:\My\Dev\snippets\eslint\node_modules\commonjseverywhere\lib\traverse-dependencies.js:109:24)
    at Controller.__execute (c:\My\Dev\snippets\eslint\node_modules\estraverse\estraverse.js:313:31)
    at Controller.replace (c:\My\Dev\snippets\eslint\node_modules\estraverse\estraverse.js:488:27)
    at Object.replace (c:\My\Dev\snippets\eslint\node_modules\estraverse\estraverse.js:556:27)
    at Object.module.exports [as traverseDependencies] (c:\My\Dev\snippets\eslint\node_modules\commonjs-everywhere\lib\traverse-dependencies.js:94:18)
    at build (c:\My\Dev\snippets\eslint\node_modules\commonjs-everywhere\lib\command.js:149:31)
    at startBuild (c:\My\Dev\snippets\eslint\node_modules\commonjs-everywhere\lib\command.js:220:17)
    at Object.<anonymous> (c:\My\Dev\snippets\eslint\node_modules\commonjs-everywhere\lib\command.js:271:5)
    at Object.<anonymous> (c:\My\Dev\snippets\eslint\node_modules\commonjs-everywhere\lib\command.js:282:3)

I added --alias fs: and --ignore-missing (as stated in michaelficarra/commonjs-everywhere#80).

16:56:50 {master * u=} [/c/My/Dev/snippets/eslint]$ node_modules/.bin/cjsify \
>     --ignore-missing \
>     --minify \
>     --export eslint \
>     --inline-sources --source-map $bundleFile.map \
>     --alias fs: \
>     --alias /lib/load-rules.js:/lib/rules/index.js \
>     lib/eslint.js > $bundleFile

c:\My\Dev\snippets\eslint\node_modules\commonjs-everywhere\lib\command.js:167
        throw e;
              ^
illegal require: argument of require must be a constant string
  `require(path.join(rulesDir, file))`
  in c:\My\Dev\snippets\eslint\lib\load-rules.js:14:35

Any hint?

/cc @michaelficarra

Contributor

albertosantini commented Nov 27, 2013

The scripts changelog, test and lint work correctly in Git Bash (1.8.4) on Windows (7-8/x64).

npm run bundle output:

> eslint@0.1.3 bundle c:\My\Dev\snippets\eslint
> bash scripts/bundle.sh


c:\My\Dev\snippets\eslint\node_modules\commonjs-everywhere\lib\command.js:167
        throw e;
              ^
Error: Core module "fs" has not yet been ported to the browser
    at resolvePath (c:\My\Dev\snippets\eslint\node_modules\commonjs-everywhere\lib\relative-resolve.js:29:15)
    at module.exports (c:\My\Dev\snippets\eslint\node_modules\commonjs-everywhere\lib\relative-resolve.js:61:16)
    at Controller.estraverse.replace.enter (c:\My\Dev\snippets\eslint\node_modules\commonjseverywhere\lib\traverse-dependencies.js:109:24)
    at Controller.__execute (c:\My\Dev\snippets\eslint\node_modules\estraverse\estraverse.js:313:31)
    at Controller.replace (c:\My\Dev\snippets\eslint\node_modules\estraverse\estraverse.js:488:27)
    at Object.replace (c:\My\Dev\snippets\eslint\node_modules\estraverse\estraverse.js:556:27)
    at Object.module.exports [as traverseDependencies] (c:\My\Dev\snippets\eslint\node_modules\commonjs-everywhere\lib\traverse-dependencies.js:94:18)
    at build (c:\My\Dev\snippets\eslint\node_modules\commonjs-everywhere\lib\command.js:149:31)
    at startBuild (c:\My\Dev\snippets\eslint\node_modules\commonjs-everywhere\lib\command.js:220:17)
    at Object.<anonymous> (c:\My\Dev\snippets\eslint\node_modules\commonjs-everywhere\lib\command.js:271:5)
    at Object.<anonymous> (c:\My\Dev\snippets\eslint\node_modules\commonjs-everywhere\lib\command.js:282:3)

I added --alias fs: and --ignore-missing (as stated in michaelficarra/commonjs-everywhere#80).

16:56:50 {master * u=} [/c/My/Dev/snippets/eslint]$ node_modules/.bin/cjsify \
>     --ignore-missing \
>     --minify \
>     --export eslint \
>     --inline-sources --source-map $bundleFile.map \
>     --alias fs: \
>     --alias /lib/load-rules.js:/lib/rules/index.js \
>     lib/eslint.js > $bundleFile

c:\My\Dev\snippets\eslint\node_modules\commonjs-everywhere\lib\command.js:167
        throw e;
              ^
illegal require: argument of require must be a constant string
  `require(path.join(rulesDir, file))`
  in c:\My\Dev\snippets\eslint\lib\load-rules.js:14:35

Any hint?

/cc @michaelficarra

@michaelficarra

This comment has been minimized.

Show comment
Hide comment
@michaelficarra

michaelficarra Nov 27, 2013

Member

@albertosantini: I'm not sure what to tell you; it builds fine for me after fixing #384. Where is fs being required?

Member

michaelficarra commented Nov 27, 2013

@albertosantini: I'm not sure what to tell you; it builds fine for me after fixing #384. Where is fs being required?

@michaelficarra

This comment has been minimized.

Show comment
Hide comment
@michaelficarra

michaelficarra Nov 27, 2013

Member

Hmm, I think I know what it is. Can you add the --verbose flag (and remove the other options you added) and send me the output? I bet it's a CJSE issue with aliasing and path delimiters.

Member

michaelficarra commented Nov 27, 2013

Hmm, I think I know what it is. Can you add the --verbose flag (and remove the other options you added) and send me the output? I bet it's a CJSE issue with aliasing and path delimiters.

@albertosantini

This comment has been minimized.

Show comment
Hide comment
@albertosantini

albertosantini Nov 27, 2013

Contributor

I suppose in load-rules.js.

I tried it also with commonjs-everywhere 0.9.4.

Getting the verbose output...

> eslint@0.1.3 bundle c:\My\Dev\snippets\eslint
> bash scripts/bundle.sh

required "esprima" from "/lib/eslint.js"
required "estraverse" from "/lib/eslint.js"
required "escope" from "/lib/eslint.js"
required "../conf/environments.json" from "/lib/eslint.js"
required "./rules" from "/lib/eslint.js"
required "./util" from "/lib/eslint.js"
required "./rule-context" from "/lib/eslint.js"
required "events" from "/lib/eslint.js"
required "./conf/eslint.json" from "/lib/eslint.js"
required "./load-rules" from "/lib/rules.js"
required "fs" from "/lib/load-rules.js"

c:\My\Dev\snippets\eslint\node_modules\commonjs-everywhere\lib\command.js:167
        throw e;
              ^
Error: Core module "fs" has not yet been ported to the browser
    at resolvePath (c:\My\Dev\snippets\eslint\node_modules\commonjs-everywhere\lib\relative-resolve.js:29:15)
    at module.exports (c:\My\Dev\snippets\eslint\node_modules\commonjs-everywhere\lib\relative-resolve.js:61:16)
    at Controller.estraverse.replace.enter (c:\My\Dev\snippets\eslint\node_modules\commonjs-everywhere\lib\traverse-dependencies.js:109:24)
    at Controller.__execute (c:\My\Dev\snippets\eslint\node_modules\estraverse\estraverse.js:313:31)
    at Controller.replace (c:\My\Dev\snippets\eslint\node_modules\estraverse\estraverse.js:488:27)
    at Object.replace (c:\My\Dev\snippets\eslint\node_modules\estraverse\estraverse.js:556:27)
    at Object.module.exports [as traverseDependencies] (c:\My\Dev\snippets\eslint\node_modules\commonjs-everywhere\lib\traverse-dependencies.js:94:18)
    at build (c:\My\Dev\snippets\eslint\node_modules\commonjs-everywhere\lib\command.js:149:31)
    at startBuild (c:\My\Dev\snippets\eslint\node_modules\commonjs-everywhere\lib\command.js:220:17)
    at Object.<anonymous> (c:\My\Dev\snippets\eslint\node_modules\commonjs-everywhere\lib\command.js:271:5)
    at Object.<anonymous> (c:\My\Dev\snippets\eslint\node_modules\commonjs-everywhere\lib\command.js:282:3)

The command executed was the following:

node_modules/.bin/cjsify \
    --verbose \
    --minify \
    --export eslint \
    --inline-sources --source-map $bundleFile.map \
    --alias /lib/load-rules.js:/lib/rules/index.js \
    lib/eslint.js > $bundleFile
Contributor

albertosantini commented Nov 27, 2013

I suppose in load-rules.js.

I tried it also with commonjs-everywhere 0.9.4.

Getting the verbose output...

> eslint@0.1.3 bundle c:\My\Dev\snippets\eslint
> bash scripts/bundle.sh

required "esprima" from "/lib/eslint.js"
required "estraverse" from "/lib/eslint.js"
required "escope" from "/lib/eslint.js"
required "../conf/environments.json" from "/lib/eslint.js"
required "./rules" from "/lib/eslint.js"
required "./util" from "/lib/eslint.js"
required "./rule-context" from "/lib/eslint.js"
required "events" from "/lib/eslint.js"
required "./conf/eslint.json" from "/lib/eslint.js"
required "./load-rules" from "/lib/rules.js"
required "fs" from "/lib/load-rules.js"

c:\My\Dev\snippets\eslint\node_modules\commonjs-everywhere\lib\command.js:167
        throw e;
              ^
Error: Core module "fs" has not yet been ported to the browser
    at resolvePath (c:\My\Dev\snippets\eslint\node_modules\commonjs-everywhere\lib\relative-resolve.js:29:15)
    at module.exports (c:\My\Dev\snippets\eslint\node_modules\commonjs-everywhere\lib\relative-resolve.js:61:16)
    at Controller.estraverse.replace.enter (c:\My\Dev\snippets\eslint\node_modules\commonjs-everywhere\lib\traverse-dependencies.js:109:24)
    at Controller.__execute (c:\My\Dev\snippets\eslint\node_modules\estraverse\estraverse.js:313:31)
    at Controller.replace (c:\My\Dev\snippets\eslint\node_modules\estraverse\estraverse.js:488:27)
    at Object.replace (c:\My\Dev\snippets\eslint\node_modules\estraverse\estraverse.js:556:27)
    at Object.module.exports [as traverseDependencies] (c:\My\Dev\snippets\eslint\node_modules\commonjs-everywhere\lib\traverse-dependencies.js:94:18)
    at build (c:\My\Dev\snippets\eslint\node_modules\commonjs-everywhere\lib\command.js:149:31)
    at startBuild (c:\My\Dev\snippets\eslint\node_modules\commonjs-everywhere\lib\command.js:220:17)
    at Object.<anonymous> (c:\My\Dev\snippets\eslint\node_modules\commonjs-everywhere\lib\command.js:271:5)
    at Object.<anonymous> (c:\My\Dev\snippets\eslint\node_modules\commonjs-everywhere\lib\command.js:282:3)

The command executed was the following:

node_modules/.bin/cjsify \
    --verbose \
    --minify \
    --export eslint \
    --inline-sources --source-map $bundleFile.map \
    --alias /lib/load-rules.js:/lib/rules/index.js \
    lib/eslint.js > $bundleFile
@michaelficarra

This comment has been minimized.

Show comment
Hide comment
@michaelficarra

michaelficarra Nov 27, 2013

Member

Yep, that's what I thought. For some reason, the --alias /lib/load-rules.js:/lib/rules/index.js is not working for you, but is working for me. Can you try to produce a minimal test case and open a CJSE issue?

Member

michaelficarra commented Nov 27, 2013

Yep, that's what I thought. For some reason, the --alias /lib/load-rules.js:/lib/rules/index.js is not working for you, but is working for me. Can you try to produce a minimal test case and open a CJSE issue?

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Nov 28, 2013

Member

FWIW, the main build system is now ShellJS, and that works everywhere for the basic testing/linting. I'll leave this open to track inclusion of the CJSE issue.

Member

nzakas commented Nov 28, 2013

FWIW, the main build system is now ShellJS, and that works everywhere for the basic testing/linting. I'll leave this open to track inclusion of the CJSE issue.

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Jan 11, 2014

Member

@michaelficarra are you planning on addressing this soon?

Member

nzakas commented Jan 11, 2014

@michaelficarra are you planning on addressing this soon?

@michaelficarra

This comment has been minimized.

Show comment
Hide comment
@michaelficarra

michaelficarra Jan 11, 2014

Member

@nzakas: Yep. I have a few CSR issues I plan on addressing first, followed by this.

Member

michaelficarra commented Jan 11, 2014

@nzakas: Yep. I have a few CSR issues I plan on addressing first, followed by this.

@nzakas nzakas closed this in 6a373bd Jan 20, 2014

nzakas added a commit that referenced this issue Jan 20, 2014

Merge pull request #551 from eslint/browserify
Build: Use browserify to create browser-ready ESLint (fixes #119)

@eslint eslint bot locked and limited conversation to collaborators Feb 7, 2018

@eslint eslint bot added the archived due to age label Feb 7, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.