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

Windows support #56

Closed
wants to merge 31 commits into from
Closed

Windows support #56

wants to merge 31 commits into from

Conversation

iiegor
Copy link
Contributor

@iiegor iiegor commented Sep 7, 2015

TODO:

  • Compatibily with Mac OS X and Windows.
  • CSS fixes.
  • Clear built-in command added.
  • Improve install/build scripts.
  • Proper source files structure/organization.

Images:

Windows 7
alt

Mac OS X El Capitán
alt

This solves #24

Note: Don't merge, this is not ready yet!

@G07cha
Copy link
Contributor

G07cha commented Sep 7, 2015

Awesome, you have done a lot of work! I think we need some time to test compatibility with Windows and OS X becouse a lot of things changed.

'watch',
$.shell.task('PATH=node_modules/.bin:$PATH electron .')
'clean',
['typescript', 'sass', 'react']
Copy link
Contributor

Choose a reason for hiding this comment

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

This is new usage for default task that makes watch task unused, should we delete it then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, you can use the watch task by running: gulp watch, It's better to set by default the build task.

At the moment I removed $.shell.task('PATH=node_modules/.bin:$PATH electron .') because this doesn't work on windows

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand but shell task don't added to gulp.watch so this may don't work on OS X

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. I think for now I will do a simple check of platform, if the platform is darwin then the shell.task will be added. This until I found a better way or why doesn't work on windows 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

We can use gulp-if for this, if you too busy with Windows compatibility I can do deeper research and implement functional that you said.

@iiegor
Copy link
Contributor Author

iiegor commented Sep 7, 2015

The last commit changes a lot of things, check it!

Also I'm getting a strange TypeError:

canBeDecorated(): boolean

Uncaught TypeError: (intermediate value).isApplicable is not a function

File: Invocation.ts Line: 129 (Commented with FIX ME)

The error says that can't check properly If the invocation can be decorated and also this produces a strange behavior and duplicates the command output but if I return false (canBeDecorated:false) it works fine... can you check this @ShockOne? Thanks!

@vlad-shatskyi
Copy link
Contributor

@iiegor, DecoratorList is an array of classes form dynamically loaded list of files. Likely, the loading doesn't happen properly on Windows. I suspect this line. https://github.com/shockone/black-screen/blob/master/src/decorators/List.ts#L18-L18

@G07cha
Copy link
Contributor

G07cha commented Sep 8, 2015

@iiegor, ping me when it's become ready for testing

@iiegor
Copy link
Contributor Author

iiegor commented Sep 8, 2015

@G07cha 👌
Fixed. Another problem I found is that the canBeDecorated() is called 2 times, is this right?

I also added a simple argument to the packaging script, --prune, this avoid packaging dev dependencies

@iiegor
Copy link
Contributor Author

iiegor commented Sep 8, 2015

With the last commit I think this is almost done. I need to check if all is working in both platforms as it should and fix minimal things.

Check this @ShockOne @G07cha

@@ -124,4 +172,4 @@ gulp.task('default', function () {
'watch',
$.shell.task('PATH=node_modules/.bin:$PATH electron .')
Copy link
Contributor

Choose a reason for hiding this comment

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

Produces an error on OS X 10.10.5
screen shot 2015-09-08 at 5 12 31 pm
If replace electron . with electron ./dist app starting succesfully

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

@G07cha
Copy link
Contributor

G07cha commented Sep 8, 2015

That's all for OS X from me, I'll check on Windows in a few hours.

@vlad-shatskyi
Copy link
Contributor

Hey, @iiegor. Great job you've done here! However, I'm afraid that BufferedProcess won't be able to replace child_pty. You see, some programs check if their output is a PTY, for example vim, or even ls. I would really like to remove child_pty, but we must in some or another way convince programs they're being run in a true terminal.

I'll review the rest of changes, but please update it against the latest master.

@iiegor
Copy link
Contributor Author

iiegor commented Sep 9, 2015

@jerch
echo Hello world! outputs:
�[?25l �[2K�[0mHello world! �[2K �[1G�[?25h

@jerch
Copy link

jerch commented Sep 9, 2015

@iiegor Thats in data.toString()? Cant work with that, coz � seems to be a replacement char by github, idk. Maybe as hex encoded?

@iiegor
Copy link
Contributor Author

iiegor commented Sep 9, 2015

@jerch Yes, data.toString() outputs that code.
The output from the browser: alt

@jerch
Copy link

jerch commented Sep 9, 2015

@iiegor : Almost there :D Could you do it once with something like this: console.log(data.toString().split('').map(function(el){return el.charCodeAt(0)}))

@iiegor
Copy link
Contributor Author

iiegor commented Sep 9, 2015

@jerch [27, 91, 63, 50, 53, 108, 13, 27, 91, 50, 75, 27, 91, 48, 109, 104, 101, 108, 108, 111, 13, 10, 27, 91, 50, 75, 13, 27, 91, 49, 71, 27, 91, 63, 50, 53, 104] 😄

@jerch
Copy link

jerch commented Sep 9, 2015

@iiegor Weird, it seems to be correct. Ill dig into this later. Thanks.

@vlad-shatskyi
Copy link
Contributor

@jerch please note that this branch uses version 1.0.2 of the parser.

@jerch
Copy link

jerch commented Sep 9, 2015

@ShockOne Yeah but this should not make any difference for the shown ANSI codes. Hmm I need to test this myself in windows -.-

"preinstall": "npm prune",
"install": "bower prune; bower install",
"start": "gulp",
"package-windows": "electron-packager ./dist \"Black Screen\" --prune --overwrite --platform=\"win32\" --arch=\"ia32\" --version=\"0.30.0\" --out=\"./.build\" --icon=\"./static/icon.icns\"",
Copy link
Contributor

Choose a reason for hiding this comment

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

As I know Windows apps requires icons in .ico format. Source.

@jerch
Copy link

jerch commented Sep 10, 2015

Ok trying to get my hands dirty on the windows port. After switching to node v12 it compiles and npm start shows no errors. But no app window is showing up. Did I miss anything?

@iiegor
Copy link
Contributor Author

iiegor commented Sep 10, 2015

Nope, all correct. Run electron ./dist

@jerch
Copy link

jerch commented Sep 10, 2015

It says: "Uncaught Error: Module did not self-register.", source: ATOM_SHELL_ASAR.js (137)
The window keeps blank. Any clues?
Sorry I am not used to node stuff under windows, kinda always feels alien.

Oh btw using Windows10 here.

@iiegor
Copy link
Contributor Author

iiegor commented Sep 10, 2015

@jerch Follow this steps:
cd node_modules/partty
HOME=~/.electron-gyp node-gyp rebuild --target=0.30.0 --arch=ia32 --dist-url=https://atom.io/download/atom-shell

This compiles the module as a native module.
Now go to the root again and run gulp build and then electron ./dist

Edit: Tested on Windows 7 and Windows 10

@jerch
Copy link

jerch commented Sep 10, 2015

Sweet its running. Thanks :)

@iiegor
Copy link
Contributor Author

iiegor commented Sep 10, 2015

I was following the pty.js repo these days and today I created a PR that fix the errors when compiling on windows so if the PR is accepted the windows support is one step closer.

I think must close this PR because contains changes that probably you don't want like the project restructuration, branch conflicts and other things I made... If you want I can create a new PR more clean with only the necessary changes.


Dev binary: https://www.dropbox.com/s/mx9qblhsdc3jqkf/Black%20Screen-win32-ia32.zip?dl=0

@vlad-shatskyi
Copy link
Contributor

@iiegor proposing the fix to the upstream repository is the right thing for sure.

There are a lot of useful changes in this branch, it would be a pity if the effort you put into making them was for nothing. If you created a separate PR for each such change, it would be awesome. I know it's a lot to ask, so, if you don't feel like that, I'll do it myself, but then the commits won't be marked as made by you. :)

@jerch
Copy link

jerch commented Sep 11, 2015

@iiegor Seems I cant get anything out of the pty in black-screen/windows. Tried it with pty.js, pty2.js and partty, all keep silent within black-screen but work with direct node/javascript access. Even changed to x64 in between, same results. Also tried "printf"-debugging in Invocation.ts, the app dies as soon as the pty layer is involved. Changing the electron version didnt help either.
Coming from Unix/Linux I am not experienced enough to solve this on my own. Sorry @iiegor

Btw with the direct access of any of the ptys with a small node script the parser works as expected and handles the ANSI codes correct. Not much of a help yet but indicating that the problem derives from somewhere else.

@jerch
Copy link

jerch commented Sep 11, 2015

@iiegor With serveral changes I got it working at least up to the parser instructions:

  • appending '\r' to the "cmd.exe /s /c" + args call
  • data in this.command.on('data') callback is already a string, .toString() doesnt work for my setup

Also I had to comment out serveral things, it seems my setup stops at random occasions (example: Parser::inst_p got a string like 'Volume in drive C: ...' but printed only 'Volum' in the output window - seems quite broken to me, maybe at react or electron level idk.
None if my half working tests showed the behavior of the remaining ANSI codes. Maybe it is related to the additional .toString() conversion?

Last tested with node v0.12, electron v0.30, ia32 and VS2010 and the original pty.js on Win10

@jerch
Copy link

jerch commented Sep 11, 2015

@iiegor Ok I gave finally up with compiling it myself and played with your binary version.

@stripAnsi:
The stripAnsi in Invocation.ts is not necessary (in fact it does what it says - no colored output for ls --color anymore and removes all the other fancy ANSI stuff). If you remove it a bug in Buffer.ts Buffer::renderRow will prevent the output from showing up - row is sometimes null, a quickfix would be to insert a line like row = row || []; right at the beginning of the method.
After those changes everything works even with colors.

@iiegor
Copy link
Contributor Author

iiegor commented Sep 11, 2015

@jerch Sorry for the late response. It's awesome what you have done!
You're right, the row is sometimes null... I tried the quickfix and works almost perfect except I'm getting a strange character at the end 😕 Do you know what can be?
alt
Thanks for your help!

@ShockOne I will do that when the PR (chjj/pty.js#133) is accepted 👌

@jerch
Copy link

jerch commented Sep 11, 2015

@iiegor lmao, I didnt even notice that but got it too. It is related to the quickfix, with if (!row) return; instead of the other fix it is gone.
Well it is just another cosmetique quickfix and might have other side effects. Since I only can test it with your binary version atm someone with a working buildchain on windows should find a real solution for the row===null case. The source of this even might point to some false assumptions on caller level deeper in the code.

@iiegor iiegor closed this Sep 12, 2015
@onbjerg
Copy link

onbjerg commented Sep 14, 2015

@iiegor What happened?

@iiegor
Copy link
Contributor Author

iiegor commented Sep 14, 2015

@onbjerg download the actual source code, run npm install & bower install, build the source with npm start and follow #129 (comment). With the last commit it is supposed to run correctly on windows 😄

@onbjerg
Copy link

onbjerg commented Sep 14, 2015

Oh, sweet! Thanks @iiegor 💃

@jazzzz
Copy link

jazzzz commented Nov 4, 2015

I still have build errors on Windows 10, is there something I should do to fix it?

Building the projects in this solution one at a time. To enable parallel build, please add the "/m" switch.
  Agent.cc
  AgentAssert.cc
  ConsoleInput.cc
  Coord.cc
  EventLoop.cc
  NamedPipe.cc
  SmallRect.cc
  Terminal.cc
  Win32Console.cc
..\..\..\deps\winpty\agent\Terminal.cc(60): error C3688: invalid literal suffix 'CSI'; literal operator or literal operator template 'operator
""CSI' not found [D:\prog\black-screen\node_modules\ptyw.js\build\deps\winpty\winpty-agent.vcxproj]
..\..\..\deps\winpty\agent\Terminal.cc(59): warning C4390: ';': empty controlled statement found; is this the intent? [D:\prog\black-screen\nod
e_modules\ptyw.js\build\deps\winpty\winpty-agent.vcxproj]
..\..\..\deps\winpty\agent\Terminal.cc(165): error C3688: invalid literal suffix 'CSI'; literal operator or literal operator template 'operator
 ""CSI' not found [D:\prog\black-screen\node_modules\ptyw.js\build\deps\winpty\winpty-agent.vcxproj]
..\..\..\deps\winpty\agent\Terminal.cc(165): error C2664: 'int sprintf(char *const ,const char *const ,...)': cannot convert argument 2 from 'i
nt' to 'const char *const ' [D:\prog\black-screen\node_modules\ptyw.js\build\deps\winpty\winpty-agent.vcxproj]
  ..\..\..\deps\winpty\agent\Terminal.cc(165): note: Conversion from integral type to pointer type requires reinterpret_cast, C-style cast or f
  unction-style cast
..\..\..\deps\winpty\agent\Terminal.cc(190): error C3688: invalid literal suffix 'CSI'; literal operator or literal operator template 'operator
 ""CSI' not found [D:\prog\black-screen\node_modules\ptyw.js\build\deps\winpty\winpty-agent.vcxproj]
..\..\..\deps\winpty\agent\Terminal.cc(190): error C2664: 'int sprintf(char *const ,const char *const ,...)': cannot convert argument 2 from 'i
nt' to 'const char *const ' [D:\prog\black-screen\node_modules\ptyw.js\build\deps\winpty\winpty-agent.vcxproj]
  ..\..\..\deps\winpty\agent\Terminal.cc(190): note: Conversion from integral type to pointer type requires reinterpret_cast, C-style cast or f
  unction-style cast
  main.cc
  DebugClient.cc
  winpty.cc
  DebugClient.cc
..\..\..\deps\winpty\libwinpty\winpty.cc(139): error C3688: invalid literal suffix 'AGENT_EXE'; literal operator or literal operator template '
operator ""AGENT_EXE' not found [D:\prog\black-screen\node_modules\ptyw.js\build\deps\winpty\winpty.vcxproj]
gyp ERR! build error
gyp ERR! stack Error: `C:\Program Files (x86)\MSBuild\14.0\bin\msbuild.exe` failed with exit code: 1
gyp ERR! stack     at ChildProcess.onExit (C:\Users\LPE\AppData\Roaming\npm\node_modules\npm\node_modules\node-gyp\lib\build.js:270:23)
gyp ERR! stack     at emitTwo (events.js:87:13)
gyp ERR! stack     at ChildProcess.emit (events.js:172:7)
gyp ERR! stack     at Process.ChildProcess._handle.onexit (internal/child_process.js:200:12)
gyp ERR! System Windows_NT 10.0.10576
gyp ERR! command "C:\\Program Files (x86)\\nodejs\\node.exe" "C:\\Users\\LPE\\AppData\\Roaming\\npm\\node_modules\\npm\\node_modules\\node-gyp\\bin\\node-gyp.js" "rebuild"
gyp ERR! cwd D:\prog\black-screen\node_modules\ptyw.js
gyp ERR! node -v v5.0.0
gyp ERR! node-gyp -v v3.0.3
gyp ERR! not ok
npm ERR! Windows_NT 10.0.10576
npm ERR! argv "C:\\Program Files (x86)\\nodejs\\node.exe" "C:\\Users\\LPE\\AppData\\Roaming\\npm\\node_modules\\npm\\bin\\npm-cli.js" "i"
npm ERR! node v5.0.0
npm ERR! npm  v3.3.10
npm ERR! code ELIFECYCLE

npm ERR! ptyw.js@0.3.2 install: `node-gyp rebuild`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the ptyw.js@0.3.2 install script 'node-gyp rebuild'.
npm ERR! Make sure you have the latest version of node.js and npm installed.
npm ERR! If you do, this is most likely a problem with the ptyw.js package,
npm ERR! not with npm itself.
npm ERR! Tell the author that this fails on your system:
npm ERR!     node-gyp rebuild
npm ERR! You can get their info via:
npm ERR!     npm owner ls ptyw.js
npm ERR! There is likely additional logging output above.

npm ERR! Please include the following file with any support request:
npm ERR!     D:\prog\black-screen\node_modules\npm-debug.log

@TheInitializer
Copy link

woah, great work! Hope this eventually gets finished

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.

9 participants