Standalone windows patch #129

iiegor · 2015-09-12T00:25:41Z

No description provided.

Conflicts: src/Aliases.ts src/CommandExecutor.ts src/Invocation.ts

vlad-shatskyi · 2015-09-12T21:54:33Z

src/CommandExecutor.ts

@@ -51,6 +51,12 @@ class BuiltInCommandExecutionStrategy extends CommandExecutionStrategy {
 class SystemFileExecutionStrategy extends CommandExecutionStrategy {
    startExecution() {
        return new Promise((resolve, reject) => {


The purpose of the strategy classes is to replace conditions with polymorphism. Please create a separate strategy for Windows. You might want to extract the common pieces into an intermediate super class.

I've refactored the way we choose a strategy to make it easier to extend. Sorry, if it created conflicts.

Conflicts: src/Aliases.ts src/Buffer.ts src/Command.ts

iiegor · 2015-09-13T11:28:55Z

Done, check now @ShockOne

vlad-shatskyi · 2015-09-13T11:32:48Z

src/CommandExecutor.ts

@@ -63,6 +64,47 @@ class SystemFileExecutionStrategy extends CommandExecutionStrategy {
    }
 }

+class WindowsSystemFileExecutionStrategy extends CommandExecutionStrategy {
+    static canExecute(command: string): Promise<boolean> {
+        return new Promise(resolve => Utils.getExecutablesInPaths().then(executables => resolve(_.include(executables, command))));


I guess this should be

return new Promise(resolve => resolve(Utils.isWindows()));

iiegor · 2015-09-13T20:35:56Z

Can you fix #129 (diff) for me? I can't find a good way to do that and also I find this like there are a lot of functions which doesn't need to be inside a class.

Regarding the enum suggestion (#129 (diff)), why would you like to do something like that? I mean, I see this also like unecessary code for a thing that can be done with a single function.

vlad-shatskyi · 2015-09-14T05:30:54Z

@iiegor, I'll do that in the evening.

Regarding enums, maybe it's just me, but I thought it's a common practice to avoid string literals comparison. With enums you always know the list of the values that exist instead of guessing whether it's 'win', 'Win', 'windows', or something else. And the program doesn't compile if you misspell the word. Anyway, it's not important, if you don't agree, I don't insist.

vlad-shatskyi · 2015-09-14T07:32:03Z

@iiegor I figured I can merge and then change the hierarchy on my own.

Also, I want you to know that this pull request is the most valuable contribution so far. Thank you very much. May I ask if you plan to continue helping the project?

Standalone windows patch

iiegor · 2015-09-14T09:07:39Z

@ShockOne Yes I will, love this project 😄

Edit: Oh, I forgot to tell you that windows users will need to run: HOME=~/.electron-gyp node-gyp rebuild --msvs_version="2013" --target=0.30.0 --arch=ia32 --dist-url=https://atom.io/download/atom-shell inside the folder node_modules\ptyw.js to build ptyw.js as a native module (is very important to do this with msvs_version=2013 if not will fail) or simply electron-rebuild but in my case electron-rebuild fails.

vlad-shatskyi · 2015-09-14T11:59:46Z

@iiegor we need to figure out how to make that happen for them automatically.

iiegor · 2015-09-14T12:43:49Z

@ShockOne hmm, I'm thinking on something like atom uses, see here and move all the scripts there, so then we can check for the platform easily and apply the right commands for each platform because for example the command above won't work in mac/linux.

onbjerg · 2015-09-14T22:45:36Z

@iiegor Does not work on Windows 10.

redacted@redacted /d/projects/bls/node_modules/ptyw.js (master)
$ HOME=~/.electron-gyp node-gyp rebuild --msvs_version="2013" --target=0.30.0 --arch=ia32 --python=/d/Python/2.7/python
.exe --dist-url=https://atom.io/download/atom-shell
gyp info it worked if it ends with ok
gyp info using node-gyp@3.0.3
gyp info using node@0.12.7 | win32 | ia32
gyp info spawn d:/Python/2.7/python.exe
gyp info spawn args [ 'c:\\Users\\nordbjerg\\AppData\\Roaming\\npm\\node_modules\\node-gyp\\gyp\\gyp_main.py',
gyp info spawn args   'binding.gyp',
gyp info spawn args   '-f',
gyp info spawn args   'msvs',
gyp info spawn args   '-G',
gyp info spawn args   'msvs_version=2013',
gyp info spawn args   '-I',
gyp info spawn args   'd:\\projects\\bls\\node_modules\\ptyw.js\\build\\config.gypi',
gyp info spawn args   '-I',
gyp info spawn args   'c:\\Users\\nordbjerg\\AppData\\Roaming\\npm\\node_modules\\node-gyp\\addon.gypi',
gyp info spawn args   '-I',
gyp info spawn args   'c:\\Users\\nordbjerg\\.electron-gyp\\.node-gyp\\0.30.0\\common.gypi',
gyp info spawn args   '-Dlibrary=shared_library',
gyp info spawn args   '-Dvisibility=default',
gyp info spawn args   '-Dnode_root_dir=c:\\Users\\nordbjerg\\.electron-gyp\\.node-gyp\\0.30.0',
gyp info spawn args   '-Dnode_gyp_dir=c:\\Users\\nordbjerg\\AppData\\Roaming\\npm\\node_modules\\node-gyp',
gyp info spawn args   '-Dnode_lib_file=node.lib',
gyp info spawn args   '-Dmodule_root_dir=d:\\projects\\bls\\node_modules\\ptyw.js',
gyp info spawn args   '--depth=.',
gyp info spawn args   '--no-parallel',
gyp info spawn args   '--generator-output',
gyp info spawn args   'd:\\projects\\bls\\node_modules\\ptyw.js\\build',
gyp info spawn args   '-Goutput_dir=.' ]
gyp info spawn C:\Windows\Microsoft.NET\Framework\v4.0.30319\msbuild.exe
gyp info spawn args [ 'build/binding.sln',
gyp info spawn args   '/clp:Verbosity=minimal',
gyp info spawn args   '/nologo',
gyp info spawn args   '/p:Configuration=Release;Platform=Win32' ]
Building the projects in this solution one at a time. To enable parallel build, please add the "/m" switch.
d:\projects\bls\node_modules\ptyw.js\build\deps\winpty\winpty-agent.vcxproj(20,3): error MSB4019: The imported project
"d:\Microsoft.Cpp.Default.props" was not found. Confirm that the path in the <Import> declaration is correct, and that
the file exists on disk.
d:\projects\bls\node_modules\ptyw.js\build\deps\winpty\winpty.vcxproj(20,3): error MSB4019: The imported project "d:\Mi
crosoft.Cpp.Default.props" was not found. Confirm that the path in the <Import> declaration is correct, and that the fi
le exists on disk.
gyp ERR! build error
gyp ERR! stack Error: `C:\Windows\Microsoft.NET\Framework\v4.0.30319\msbuild.exe` failed with exit code: 1
gyp ERR! stack     at ChildProcess.onExit (c:\Users\nordbjerg\AppData\Roaming\npm\node_modules\node-gyp\lib\build.js:270:23)
gyp ERR! stack     at ChildProcess.emit (events.js:110:17)
gyp ERR! stack     at Process.ChildProcess._handle.onexit (child_process.js:1074:12)
gyp ERR! System Windows_NT 6.3.9600
gyp ERR! command "d:\\Program Files (x86)\\nodejs\\node.exe" "c:\\Users\\nordbjerg\\AppData\\Roaming\\npm\\node_modules\\node-gyp\\bin\\node-gyp.js" "rebuild" "--msvs_version=2013" "--target=0.30.0" "--arch=ia32" "--python=d:/Python/2.7/python.exe" "--dist-url=https://atom.io/download/atom-shell"
gyp ERR! cwd d:\projects\bls\node_modules\ptyw.js
gyp ERR! node -v v0.12.7
gyp ERR! node-gyp -v v3.0.3
gyp ERR! not ok

MarkTiedemann · 2015-09-14T23:17:42Z

@iiegor @onbjerg Same error here.

Note: To get it working until that point, I also had to reinstall node-gyp and downgrade my python version to 2.7.10 since node-gyp only supports python versions between 2.5.0 and 3.0.0.

vlad-shatskyi · 2015-09-14T23:21:57Z

@iiegor, well, if we have to. But before that, could you check if you can make electron-rebuild work with that parameter? It's much simpler that way.

onbjerg · 2015-09-15T00:03:36Z

@MarkTiedemann Same thing here.

iiegor · 2015-09-15T14:05:15Z

@onbjerg d:\Microsoft.Cpp.Default.props is missing, download Visual Studio 2013 and try again, please. Also, can you create an issue for this? so others users can find the solution

@ShockOne Running:

export GYP_MSVS_VERSION=2013
electron-rebuild

should work but electron-rebuild is giving me errors so if anyone can test this will be great.

onbjerg · 2015-09-15T14:27:08Z

So the Visual Studio 2013 Redist is not enough? @iiegor

iiegor · 2015-09-15T19:27:43Z

@onbjerg I don't know, I think is not enough... I have Visual Studio 2013, Python 2.7 and the latest node-gyp and works perfect, can you try with that? or try running this commands with Visual Studio 2013 installed:

export GYP_MSVS_VERSION=2013
electron-rebuild

onbjerg · 2015-09-15T21:19:00Z

I've tried exporting those variables, but I would not use black-screen if I need to have Visual Studio 2013 installed. Visual Studio bloats my system with a lot of programs that I do not need, because it installs a ton of different packages. I'm never going to use it, so I would rather not.

It is a weird issue, though, as I have no problem building and packaging my own electron projects under Windows, even though some of them also require a rebuild of some modules.

jerch · 2015-09-18T13:44:36Z

@onbjerg "...but I would not use black-screen if I need to have Visual Studio 2013 installed..."

@ALL
Is there a way to circumvent the node-gyp thingy and deliver binaries for a real distribution package later on? IMHO any unexpericened user (like me with windows and node) would never comply to the "get VS2013 beforehand" issue. As long as it is in dev space - well its normal you would need it ;)
The windows port seems to work only with one specific set of dev tools. This needs at least a very clear documentation about what to do and such.

iiegor · 2015-09-18T14:26:09Z

@jerch You mean distribution binaries for ptyw.js without compiling with node-gyp? If this is what you mean, I think it's possible, try with this pre-compiled binaries by me: https://www.dropbox.com/s/0h8kuo35r508te1/Release.zip?dl=0 (unzip this in node_modules/ptyw.js/build)

@onbjerg What are your intentions? I mean, if you want to compile yourself a binary of black-screen on windows I'm afraid you will need to have VS2013 installed or if you can wait, I will compile the respective binary files when black-screen reaches a stable release. Anyway, you can try to download the binaries (link above) of ptyw.js for windows and check if works, so you won't need to have VS2013.

Steps to build on windows

run npm install && bower install
run set GYP_MSVS_VERSION=2013 && electron-rebuild but this can fail. Another option is going to node_modules/ptyw.js and run set HOME=~/.electron-gyp && node-gyp rebuild --msvs_version="2013" --target=0.32.1 --arch=ia32 --dist-url=https://atom.io/download/atom-shell (you need VS2013)
go to the root folder of the project and run npm start to compile the project files
run electron . and should work
If you want to create a binary for windows, actually there is no script for that but this should work: electron-packager . "Black Screen" --prune --overwrite --platform="win32" --arch="ia32" --version="0.32.1" --out="./.build"

My english is not very good so if someone could put this in the README would be great.

rmobis · 2015-09-18T17:42:55Z

@iiegor When you say export, do you mean SET? Because my Windows (8.1, tried on both cmd and PowerShell) doesn't seem to have an export command/executable and (I'm pretty sure) SET is the equivalent of export on Linux, so maybe you have an alias?

The command HOME=~/.electron-gyp node-gyp rebuild --msvs_version="2013" --target=0.30.0 --arch=ia32 --dist-url=https://atom.io/download/atom-shell also doesn't seem to work on my machine, reporting that HOME isn't a command either and I couldn't exactly figure out what you meant with this one.

Also worth noting that for both electron-rebuild, electron and electron-package binaries to be ran, I had to call them from their node_modules/.bin folder, as they weren't on my PATH by default and maybe won't be for everyone else.

Either way, I followed the steps you described, using the master branch. I managed to get all steps to work (on the second step, electron-rebuild worked or at least didn't display any errors) but the final result didn't. I had some issue with fixedsticky not being found. While I was writing this I realized I hadn't ran bower install and, after doing so and restarting npm start, electron . worked just fine.

However, I still couldn't get the binary to properly work. At first, I was getting the following error (loosely translated):

Uncaught Error: %1 is not a valid Win32 application.
D:\black-screen\.build\Black Screen-win32-ia32\resources\app\node_modules\ptyw.js\build\Release\pty.node", source: ATOM_SHELL_ASAR.js (137)

My first thought was that the fact that I was running a x64 version of Node was the problem, so I decided to try the packaging command using --arch=x64. It compiled fine and when running it, it seemed to generate a slightly better result (The screen was pretty much fully rendered, I just couldn't type anything), but I still got the an error:

Uncaught Error: Invariant Violation: addComponentAsRefTo(...): Only a ReactOwner can have refs. This usually means that you're trying to add a ref to a component that doesn't have an owner (that is, was not created inside of another component's `render` method). Try rendering this component inside of a new top-level component which will hold the ref.", source: D:\black-screen\node_modules\react\lib\invariant.js

So, my last attempt was to install a x86 version of Node and rerun all the steps. Unfortunately, that still gave me the same error as the x64 version.

Either way, at least I got the unpackaged version to work.

iiegor · 2015-09-18T21:00:42Z

@rmobis Oh sorry, you are right, it's SET and regarding the react error (addComponentAsRefTo) it's because the build folder is inside the project folder and the react library is called two times, try moving the build folder outside and try again, should work fine 👌

rmobis · 2015-09-18T21:07:31Z

@iiegor Indeed, moving the .build folder outside the repo folder does work. Thanks.

jerch · 2015-09-18T22:26:08Z

@iiegor Just wondering - isn't your instruction listing above to get it build on windows related to git bash or some other unix shell windows env? IMHO HOME=~/.electron-gyp node-gyp rebuild won't work on native cmd.exe because of the unixish ENV variable syntax. (can't test it right now)

iiegor · 2015-09-18T22:40:59Z

@jerch Yes, MINGW32. For native shell run set HOME=~/.electron-gyp && node-gyp rebuild --msvs_version="2013" --target=0.32.1 --arch=ia32 --dist-url=https://atom.io/download/atom-shell, I will edit that in the comment above, thanks.

onbjerg · 2015-09-19T01:34:39Z

@iiegor I would like to contribute to the project, but I refuse to install Visual Studio 2013. I know this is not a fault in black-screen, but rather node-gyp or Micro$oft's redists. I'll never use VS13 or any VS for that matter, so I find this unnecessary. I've never had this issue before either, when rebuilding npm packages on Windows.

Anyway, I think I'm switching back to Linux in a moment anyway, so this won't be much of an issue for me.

iiegor added 4 commits September 12, 2015 01:54

🏁 Windows patch

d698363

Missing type definition

ee93490

Merge

74412b4

🏁 Windows patch

f554789

iiegor changed the title ~~Standalone windows patch from #56~~ Standalone windows patch Sep 12, 2015

iiegor added 2 commits September 12, 2015 13:51

Merge https://github.com/black-screen/black-screen into win-fix

8d81865

Conflicts: src/Aliases.ts src/CommandExecutor.ts src/Invocation.ts

🏠 Fix the home path

e4810ae

vlad-shatskyi reviewed Sep 12, 2015
View reviewed changes

iiegor added 2 commits September 13, 2015 12:50

Merge https://github.com/black-screen/black-screen into win-fix

6a7961c

Conflicts: src/Aliases.ts src/Buffer.ts src/Command.ts

🎨 Better command execution

73b6403

vlad-shatskyi reviewed Sep 13, 2015
View reviewed changes

Changes based on notes

01ef745

vlad-shatskyi closed this Sep 14, 2015

vlad-shatskyi reopened this Sep 14, 2015

vlad-shatskyi added the in progress label Sep 14, 2015

vlad-shatskyi added a commit that referenced this pull request Sep 14, 2015

Merge pull request #129 from iiegor/win-fix

0711abe

Standalone windows patch

vlad-shatskyi merged commit 0711abe into railsware:master Sep 14, 2015

vlad-shatskyi removed the in progress label Sep 14, 2015

iiegor mentioned this pull request Sep 14, 2015

Windows support #56

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Standalone windows patch #129

Standalone windows patch #129

iiegor commented Sep 12, 2015

vlad-shatskyi Sep 12, 2015

vlad-shatskyi Sep 13, 2015

iiegor commented Sep 13, 2015

vlad-shatskyi Sep 13, 2015

iiegor commented Sep 13, 2015

vlad-shatskyi commented Sep 14, 2015

vlad-shatskyi commented Sep 14, 2015

iiegor commented Sep 14, 2015

vlad-shatskyi commented Sep 14, 2015

iiegor commented Sep 14, 2015

onbjerg commented Sep 14, 2015

MarkTiedemann commented Sep 14, 2015

vlad-shatskyi commented Sep 14, 2015

onbjerg commented Sep 15, 2015

iiegor commented Sep 15, 2015

onbjerg commented Sep 15, 2015

iiegor commented Sep 15, 2015

onbjerg commented Sep 15, 2015

jerch commented Sep 18, 2015

iiegor commented Sep 18, 2015

rmobis commented Sep 18, 2015

iiegor commented Sep 18, 2015

rmobis commented Sep 18, 2015

jerch commented Sep 18, 2015

iiegor commented Sep 18, 2015

onbjerg commented Sep 19, 2015

Standalone windows patch #129

Standalone windows patch #129

Conversation

iiegor commented Sep 12, 2015

vlad-shatskyi Sep 12, 2015

Choose a reason for hiding this comment

vlad-shatskyi Sep 13, 2015

Choose a reason for hiding this comment

iiegor commented Sep 13, 2015

vlad-shatskyi Sep 13, 2015

Choose a reason for hiding this comment

iiegor commented Sep 13, 2015

vlad-shatskyi commented Sep 14, 2015

vlad-shatskyi commented Sep 14, 2015

iiegor commented Sep 14, 2015

vlad-shatskyi commented Sep 14, 2015

iiegor commented Sep 14, 2015

onbjerg commented Sep 14, 2015

MarkTiedemann commented Sep 14, 2015

vlad-shatskyi commented Sep 14, 2015

onbjerg commented Sep 15, 2015

iiegor commented Sep 15, 2015

onbjerg commented Sep 15, 2015

iiegor commented Sep 15, 2015

onbjerg commented Sep 15, 2015

jerch commented Sep 18, 2015

iiegor commented Sep 18, 2015

Steps to build on windows

rmobis commented Sep 18, 2015

iiegor commented Sep 18, 2015

rmobis commented Sep 18, 2015

jerch commented Sep 18, 2015

iiegor commented Sep 18, 2015

onbjerg commented Sep 19, 2015