Skip to content
This repository has been archived by the owner on Mar 3, 2023. It is now read-only.

Fix scripts on Windows #1054

Merged
merged 5 commits into from
Oct 31, 2013
Merged

Fix scripts on Windows #1054

merged 5 commits into from
Oct 31, 2013

Conversation

zcbenz
Copy link
Contributor

@zcbenz zcbenz commented Oct 30, 2013

This fixed some errors when running grunt, build, test and bootstrap on Windows.

This reverts commit 8742f6c.

The `node_modules/.bin/apm` is a bash script not a js script, so on
Windows `node node_modules/.bin/apm` would fail with:

```
C:\cygwin\home\zcbenz\codes\atom\node_modules\.bin\apm:2
basedir=`dirname "$0"`
        ^
SyntaxError: Unexpected token ILLEGAL
    at Module._compile (module.js:439:25)
    at Object.Module._extensions..js (module.js:474:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:312:12)
    at Function.Module.runMain (module.js:497:10)
    at startup (node.js:119:16)
    at node.js:901:3
```
@@ -24,7 +24,7 @@ var commands = [
joinCommands('cd vendor/apm', 'npm install --silent .'),
'npm install --silent vendor/apm',
echoNewLine,
'node node_modules/.bin/apm install --silent'
'node vendor/apm/bin/apm install --silent',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We really need to run it from node_modules since that is the JS version, running it out vendor runs the Coffee version.

Could this just be node_modules/atom-package-manager/bin/apm instead to work on windows?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah node_modules/atom-package-manager/bin/apm seems a better choice to me.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, so just to confirm, we should never use paths to things in node_modules/.bin since on windows they won't exist?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually node_modules/.bin/xxx would be a bash script for POSIX systems, and node_modules/.bin/xxx.cmd would be a batch file for Windows, we could use things under node_modules/.bin but the code would be platform-dependent.

spawn {cmd: 'unzip', args: [zipPath, '-d', directoryPath]}, (error) ->
rm(zipPath)
callback(error)
else
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the unzip command on windows not work?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, there is no unzip command on Windows unless you installed a third party tool.

zcbenz added a commit that referenced this pull request Oct 31, 2013
@zcbenz zcbenz merged commit 993cc75 into master Oct 31, 2013
@zcbenz zcbenz deleted the cz-fix-win32-scripts branch October 31, 2013 00:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants