-
Notifications
You must be signed in to change notification settings - Fork 921
#2313 More portable nix/bash detection to include mingw #2323
base: master
Are you sure you want to change the base?
Conversation
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.
This looks useful @robjens - thank you! I should be able to test this tomorrow, but just a few observations in the meantime:
Sorry for all the questions! 😄 |
EDIT: Had to manually delete contents of |
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? |
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? |
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. |
I've been using the fix from this PR and #2321 on Windows 8.1 and have not ran into any troubles. |
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