Skip to content
This repository has been archived by the owner on Oct 21, 2022. It is now read-only.

Better portability: build on Windows + Git Bash to run script/light.sh #2313

Open
robjens opened this issue Jan 16, 2017 · 8 comments
Open

Comments

@robjens
Copy link

robjens commented Jan 16, 2017

Currently, the script/light.sh file ran after building this solution on a Windows machine, doesn't pick up the fact that I can actually run LightTable just fine (Cannot detect a supported OS.)

At the moment, it checks first for Darwin - which I'd keep since it to has bash - but then proceeds to check Linux using uname -s as well, finally to check for Cygwin in the same fashion.

Now, I am not sure if and why Cygwin would need to run the .exe executable but I presume this to be correct. However, in my case and sense, the following would have been a more reliable way to check:

 elif hash bash 2>/dev/null; then
    CLI="${DIR}/deploy/electron/electron/electron"
elif

If Cygwin does in fact require the executable and somehow is incapable of running the shell file, it should come first. Else it could just the same code as above. It would run on any bash capable/installed shell under Windows.

Oh and note that $(expr substr ...) doesn't work here either, which is preventing it from finding out the system string to begin with. I guess, without any external commands, variable assignment and then built-in substring would be the 'cleanest' (which it isn't, use zsh for that) way: currsys=$(uname -s) && echo ${currsys:0:10} Unfortunately, bash doesn't really allow for much subscripting so I don't think nested function, variable assignment and substring is possible in 1 go.

For the record, my Git Bash reports MINGW64_NT-10.0 which makes me inclined to think that whole string isn't really reliable but I guess one could work with the MINGW bit.

I wouldn't mind doing a PR but figured I should ask first if this would be a desirable change. The used snippet is the most idiomatic and portable way of checking I could think of.

@sbauer322
Copy link
Contributor

I'm not opposed to a PR for this... As long as everything still works! 😄

@LightTable/maintainers - any concerns or objections?

@Mouvedia
Copy link
Collaborator

Mouvedia commented Jan 17, 2017

@robjens while you are at it could you check bash on Windows Subsystem for Linux?

@rundis
Copy link
Contributor

rundis commented Jan 17, 2017

No objections, as long as it works... and we are able to test that it does I'm good.

@sbauer322
Copy link
Contributor

Sounds like a PR for this would be appreciated @robjens - feel free to proceed if you so desire.

@robjens
Copy link
Author

robjens commented Feb 1, 2017

Alright, will do... cheers. I'm not sure if I'm supposed to sign a CA but I'll check. Also I noticed it didn't work out of the box as expected when I changed the script pre-build but it might have also overwritten something so I'll take care to double check before I do a PR.

@sbauer322
Copy link
Contributor

No need for a CA from us!

@robjens
Copy link
Author

robjens commented Feb 23, 2017

@sbauer322 Cool. I just built LT again on Windows and I changed the light.sh script before running build.sh for the first time. On compilation/setup finish, it tries to run script/light.sh at the very end right? It seems it may not be using the local one though, I'm guessing it uses the git cloned script ...? Since it basically notifies right after the git activity...

image

If I try to run bash script/light.sh it runs without any issue.

@sbauer322
Copy link
Contributor

sbauer322 commented Feb 25, 2017

Oh, I believe the last Cannot detect a supported OS is coming from script/build-app.sh it will need something for MINGW as well... mentioned this further here.

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

No branches or pull requests

4 participants