#2313 More portable nix/bash detection to include mingw

[robjens]

Slight enhancement to have moderately better Windows bash support. Mingw doesn't seem to have expr in its environment so the script failed to detect when run in e.g. Git Bash on Windows. This would fix it and should work on any Linux at the same time. I installed Cygwin just to ensure that the generic second condition wouldn't override Cygwin last, it doesn't. Resolves #2313

[sbauer322]

This looks useful @robjens - thank you!

I should be able to test this tomorrow, but just a few observations in the meantime:

• Does the line changed mean that bash will now be required when building on Linux? I understand bash is fairly ubiquitous on Linux, but some people might have replaced it for something else. Not an issue with this PR since the scripts already require bash...
• script/build-app.sh will also need to check for MINGW. Adding something like the following seemed to have taken care of things (but feel free to implement how you see fit):

elif [ "$(expr substr $(uname -s) 1 5)" == "MINGW" ]; then
  OS="windows"
  RESOURCES="resources"
  PLATFORM_DIR="deploy/platform/win"

• script/build-app.sh also appears to use cygwin to zip up Light Table. If so, we would likely need the same for MINGW. If you search for cygdrive, you should be able to locate the section.
• Would you mind throwing in a comment on the line to indicate that it is for mingw / Git Bash on Windows / Linux (assuming the above isn't an issue)? It won't be obvious to most why the line is different from the others.
• Could you also update the developer-install in this PR since Cygwin will no longer be the only option for Windows?

Sorry for all the questions! 😄

[sbauer322]

I seem to be experiencing the same problem as described here #2321 (comment) .

Would you be able to test what happens when you run script/build.sh from MINGW? I am currently testing this with GitBash and keep getting the script/build.sh: line 25: node_modules/.bin/grunt: No such file or directory message.

Do you have grunt or grunt.cmd in your deploy\electron\node_modules\.bin directory (or both)?

EDIT: Had to manually delete contents of deploy/node_modules in order to resolve my problem.

[sbauer322]

This seems to work just fine on Windows with GitBash... despite my initial unrelated caching problem.

For Linux, as expected, if you don't have bash installed then the script won't work. However, since the script already requires bash prior to this PR I no longer feel this is an issue for this PR.

If you could make the changes mentioned in an earlier comment I think this will be fine, @robjens.

@LightTable/maintainers anyone available such that we can get another person to QA this?

[domoran]

It seems we had the same idea on this pull request #2321.

I already made the requested adaptions to the developer docs. How shall we proceed here?

[sbauer322]

It is certainly a rare case to have multiple PRs addressing the same thing here. I don't have a preference for which one is used as long as it addresses the concerns mentioned.

It might be simplest to reduce @domoran's PR to just the developer docs and merge it at the same time as @robjens's PR is merged?

If one contributor or the other is too busy to work on their PR then we can simply take the other PR instead.

[sbauer322]

I've been using the fix from this PR and #2321 on Windows 8.1 and have not ran into any troubles.