Supporting env var assignments on Windows #316

joshuazmiller · 2016-06-29T04:57:42Z

Trying to resolve #313

joshuazmiller · 2016-06-29T05:11:26Z

Can we ignore the failed coverage test? You can't write coverage to the best of my knowledge.

levithomason · 2016-06-29T15:38:43Z

package.json

    "release:major": "ta-script npm/release.sh major",
    "release:minor": "ta-script npm/release.sh minor",
    "release:patch": "ta-script npm/release.sh patch",
    "start": "npm run docs",
    "start:local-modules": "npm run docs -- --local-modules",
    "pretest": "gulp dll",
-    "test": "NODE_ENV=test babel-node $(npm bin)/karma start build/karma.conf.babel.js",
+    "test": "cross-env NODE_ENV=test babel-node node_modules/karma start build/karma.conf.babel.js",


This should be node_modules/.bin/karma.

Details:
When you npm install packages that come with executables, they are placed in node_modules/.bin. You can get the absolute path to the npm bin with the command npm bin. NPM also adds this directory to your $PATH when you npm run ... scripts. This way you can execute scripts that you have installed. The $(npm bin) syntax is command substitution. It places the output of that command inline in another command. So, we were placing the absolute path to the npm bin directory just before /karma.

Thank you for explaining that to me. I learned something new!

levithomason · 2016-06-29T15:41:57Z

Thanks for the PR. Please reference the related issue in the description in order to close the issue when the PR is merged.

Coverage will not fail for this PR. Coverage is calculated by lines of code covered by tests. Since this PR touches no code, the coverage will not be affected. CircleCI seems to have blown up for some random reason. It should run on the next commit.

levithomason · 2016-06-30T05:36:50Z

@joshuazmiller it looks like tests are failing because they are never running in the first place. Pretty sure this is due to the karma path issue I noted here https://github.com/TechnologyAdvice/stardust/pull/316#discussion_r68971089

Also added node_module/.bin/karma

codecov-io · 2016-06-30T11:00:26Z

Current coverage is 88.30%

Merging #316 into master will not change coverage

@@             master       #316   diff @@
==========================================
  Files            62         62          
  Lines           821        821          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits            725        725          
  Misses           96         96          
  Partials          0          0

Powered by Codecov. Last updated by 8cad5a9...731d91d

joshuazmiller · 2016-06-30T11:19:42Z

It doesn't seem I have permission to manually close #313 , however I added it to my description and perhaps it will work now. I'll keep in mind to mark those in for future pull-requests.

When I run test with the .bin I get the following error:

C:\Users\Joshua\Dropbox\GitHub\stardust\node_modules.bin\karma:2
basedir=$(dirname "$(echo "$0" | sed -e 's,,/,g')")
^^^^^^^

SyntaxError: missing ) after argument list
at exports.runInThisContext (vm.js:53:16)
at Module._compile (module.js:373:25)
at Module._extensions..js (module.js:416:10)
at Object.require.extensions.(anonymous function) as .js
at Module.load (module.js:343:32)
at Function.Module._load (module.js:300:12)
at Function.Module.runMain (module.js:441:10)
at C:\Users\Joshua\Dropbox\GitHub\stardust\node_modules\babel-cli\lib_babel-node.js:160:24
at Object. (C:\Users\Joshua\Dropbox\GitHub\stardust\node_modules\babel-cli\lib_babel-node.js:161:7)
at Module._compile (module.js:409:26)

I haven't written bash scripts in years, and I pretty much can't make sense of the file to understand what's wrong.

levithomason · 2016-06-30T14:56:48Z

Ah, yes. The karma script that is running is not compatible with Windows. You would likely need to run the "karma.cmd" version instead.

We can't hard code the test script to that or it won't work on Unix machines. We'll need to figure out how to support Windows here.

Any help appreciated as I so not have a Windows machine available. I will do some digging on this though.

levithomason · 2016-07-01T03:41:56Z

It doesn't seem I have permission to manually close #313

Indeed, even if you did have permissions the best practice is to leave the issue open until the fix is merged. The description you have in this PR should take care of it!

levithomason · 2016-07-01T03:47:22Z

@joshuazmiller I just merged #320 which removes babel-node entirely. This means we no longer need any special paths to karma on the test script. I am hoping by running karma directly like this that npm runs the correct version for Windows and also for Unix.

Please update this PR with the latest master and set the test script to:

"test": "cross-env NODE_ENV=test karma start build/karma.conf.babel.js",

Let's see if tests then pass, and also if it runs locally for you.

joshuazmiller · 2016-07-01T04:58:32Z

Unfortunately, I still get errors after the merge and adding that line:

cross-env NODE_ENV=test karma start build/karma.conf.babel.js

30 06 2016 21:55:48.509:ERROR [config]: Invalid config file!
SyntaxError: Unexpected token {
at exports.runInThisContext (vm.js:53:16)
at Module._compile (module.js:373:25)
at Object.Module._extensions..js (module.js:416:10)
at Module.load (module.js:343:32)
at Function.Module._load (module.js:300:12)
at Module.require (module.js:353:17)
at require (internal/module.js:12:17)
at Object.parseConfig (C:\Users\Joshua\Dropbox\GitHub\stardust\node_modules\karma\lib\config.js:284:22)
at new Server (C:\Users\Joshua\Dropbox\GitHub\stardust\node_modules\karma\lib\server.js:57:20)
at Object.exports.run (C:\Users\Joshua\Dropbox\GitHub\stardust\node_modules\karma\lib\cli.js:243:7)
at requireCliAndRun (C:\Users\Joshua\Dropbox\GitHub\stardust\node_modules\karma-cli\bin\karma:44:16)
at C:\Users\Joshua\Dropbox\GitHub\stardust\node_modules\karma-cli\bin\karma:54:12
at C:\Users\Joshua\Dropbox\GitHub\stardust\node_modules\resolve\lib\async.js:44:21
at ondir (C:\Users\Joshua\Dropbox\GitHub\stardust\node_modules\resolve\lib\async.js:187:31)
at C:\Users\Joshua\Dropbox\GitHub\stardust\node_modules\resolve\lib\async.js:153:39
at onex (C:\Users\Joshua\Dropbox\GitHub\stardust\node_modules\resolve\lib\async.js:93:22)
npm ERR! Windows_NT 10.0.10586
npm ERR! argv "C:\Program Files\nodejs\node.exe" "C:\Users\Joshua\AppData\Roaming\npm\node_modules\npm\bin\npm-cli.js" "run" "test"
npm ERR! node v4.4.7
npm ERR! npm v3.10.2
npm ERR! code ELIFECYCLE
npm ERR! stardust@0.16.4 test: cross-env NODE_ENV=test karma start build/karma.conf.babel.js
npm ERR! Exit status 1

…into windows

levithomason · 2016-07-01T14:46:57Z

I see in your error output you are running:

npm ERR! node v4.4.7
npm ERR! npm v3.10.2

This project requires Node 6. I just added a note about this in the contributing guide last night.

Please install Node 6 and give it another go. Thanks for hanging in there through all this! I think we're close to a solution.

levithomason · 2016-07-02T19:59:42Z

@joshuazmiller did updating Node help at all?

joshuazmiller · 2016-07-03T06:38:37Z

It works now! Thanks!

levithomason · 2016-07-03T06:42:13Z

Awesome! 😸 Very glad we finally got it.

Just one last tweak and we can merge this PR. It looks like the package description got set wrong (probably after a conflict fix). It should be:

"description": "The official Semantic-UI-React integration."

Once that is updated, we can get this PR in. Thank you very much for hanging in there.

…into windows

joshuazmiller · 2016-07-03T07:53:18Z

This should do it. Thank you. I am going to get back to the List Item and Item API as soon as I can

* Supporting env var assignments on Windows * fix: change package.json to have cross-env * fix: correct the description

Supporting env var assignments on Windows

c6a78a3

joshuazmiller mentioned this pull request Jun 29, 2016

Support Windows env var assignments #313

Closed

levithomason reviewed Jun 29, 2016
View reviewed changes

Merged branch 'master' of TechnologyAdvice/stardust into windows

4c0e788

Also added node_module/.bin/karma

levithomason mentioned this pull request Jul 1, 2016

Drop babel-node from build process #320

Merged

joshuazmiller added 2 commits June 30, 2016 21:35

Merge branch 'master' into windows

5fb3a16

fix: change package.json to have cross-env

2e937c7

joshuazmiller added 2 commits June 30, 2016 22:00

Merge branch 'master' of https://github.com/TechnologyAdvice/stardust …

41cce0e

…into windows

Merge remote-tracking branch 'origin/windows' into windows

9594cf0

joshuazmiller added 2 commits July 3, 2016 00:46

Merge branch 'master' of https://github.com/TechnologyAdvice/stardust …

11963b7

…into windows

fix: correct the description

731d91d

levithomason merged commit 54eb236 into Semantic-Org:master Jul 3, 2016

jhchill666 pushed a commit to jhchill666/stardust that referenced this pull request Jul 5, 2016

Supporting env var assignments on Windows (Semantic-Org#316)

3fa4c6b

* Supporting env var assignments on Windows * fix: change package.json to have cross-env * fix: correct the description

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Supporting env var assignments on Windows #316

Supporting env var assignments on Windows #316

joshuazmiller commented Jun 29, 2016 •

edited

Loading

joshuazmiller commented Jun 29, 2016

levithomason Jun 29, 2016

joshuazmiller Jun 30, 2016

levithomason commented Jun 29, 2016

levithomason commented Jun 30, 2016

codecov-io commented Jun 30, 2016 •

edited

Loading

joshuazmiller commented Jun 30, 2016

levithomason commented Jun 30, 2016

levithomason commented Jul 1, 2016

levithomason commented Jul 1, 2016

joshuazmiller commented Jul 1, 2016

levithomason commented Jul 1, 2016 •

edited

Loading

levithomason commented Jul 2, 2016

joshuazmiller commented Jul 3, 2016

levithomason commented Jul 3, 2016

joshuazmiller commented Jul 3, 2016

Supporting env var assignments on Windows #316

Supporting env var assignments on Windows #316

Conversation

joshuazmiller commented Jun 29, 2016 • edited Loading

joshuazmiller commented Jun 29, 2016

levithomason Jun 29, 2016

Choose a reason for hiding this comment

joshuazmiller Jun 30, 2016

Choose a reason for hiding this comment

levithomason commented Jun 29, 2016

levithomason commented Jun 30, 2016

codecov-io commented Jun 30, 2016 • edited Loading

Current coverage is 88.30%

joshuazmiller commented Jun 30, 2016

levithomason commented Jun 30, 2016

levithomason commented Jul 1, 2016

levithomason commented Jul 1, 2016

joshuazmiller commented Jul 1, 2016

levithomason commented Jul 1, 2016 • edited Loading

levithomason commented Jul 2, 2016

joshuazmiller commented Jul 3, 2016

levithomason commented Jul 3, 2016

joshuazmiller commented Jul 3, 2016

joshuazmiller commented Jun 29, 2016 •

edited

Loading

codecov-io commented Jun 30, 2016 •

edited

Loading

levithomason commented Jul 1, 2016 •

edited

Loading