update shell node version to 6.3.1 #543

zaggino · 2015-12-14T04:52:25Z

I've tried doing this, but I get the following error which is not comprehensible for me:

>> Error: Command failed: C:\WINDOWS\system32\cmd.exe /s /c "bash scripts/fix-msvc.sh"
>>       2 [main] bash 3416 C:\Program Files\Git\usr\bin\bash.exe: * fatal error in forked process - fork: can't reserve memory for parent stack 0x600000 - 0x800000, (child has 0x400000 - 0x600000), Win32 error 487
>>     513 [main] bash 3416 cygwin_exception::open_stackdumpfile: Dumping stack trace to bash.exe.stackdump
>>       2 [main] bash 8080 fork: child -1 - forked process 3416 died unexpectedly, retry 0, exit code 0x100, errno 11
>> scripts/fix-msvc.sh: fork: retry: No child processes
>> 1004878 [main] bash 10932 C:\Program Files\Git\usr\bin\bash.exe: * fatal error in forked process - fork: can't reserve memory for parent stack 0x600000 - 0x800000, (child has 0x400000 - 0x600000), Win32 error 487
>> 1005294 [main] bash 10932 cygwin_exception::open_stackdumpfile: Dumping stack trace to bash.exe.stackdump
>> 1100847 [main] bash 8080 fork: child -1 - forked process 10932 died unexpectedly, retry 0, exit code 0x100, errno 11
>> scripts/fix-msvc.sh: fork: retry: No child processes
>> 3108069 [main] bash 8804 C:\Program Files\Git\usr\bin\bash.exe: * fatal error in forked process - fork: can't reserve memory for parent stack 0x600000 - 0x800000, (child has 0x400000 - 0x600000), Win32 error 487
>> 3108730 [main] bash 8804 cygwin_exception::open_stackdumpfile: Dumping stack trace to bash.exe.stackdump
>> 3207844 [main] bash 8080 fork: child -1 - forked process 8804 died unexpectedly, retry 0, exit code 0x100, errno 11
>> scripts/fix-msvc.sh: fork: retry: No child processes
>> 7224872 [main] bash 11240 C:\Program Files\Git\usr\bin\bash.exe: * fatal error in forked process - fork: can't reserve memory for parent stack 0x600000 - 0x800000, (child has 0x400000 - 0x600000), Win32 error 487
>> 7226544 [main] bash 11240 cygwin_exception::open_stackdumpfile: Dumping stack trace to bash.exe.stackdump
>> 7316396 [main] bash 8080 fork: child -1 - forked process 11240 died unexpectedly, retry 0, exit code 0x100, errno 11
>> scripts/fix-msvc.sh: fork: retry: No child processes
>> 15320804 [main] bash 11272 C:\Program Files\Git\usr\bin\bash.exe: * fatal error in forked process - fork: can't reserve memory for parent stack 0x600000 - 0x800000, (child has 0x400000 - 0x600000), Win32 error 487
>> 15321198 [main] bash 11272 cygwin_exception::open_stackdumpfile: Dumping stack trace to bash.exe.stackdump
>> 15409163 [main] bash 8080 fork: child -1 - forked process 11272 died unexpectedly, retry 0, exit code 0x100, errno 11

also, why do we need npm?

ingorichter · 2015-12-17T04:37:22Z

I don't exactly see why we need npm. Perhaps @jasonsanjose recalls why we need it.

zaggino · 2015-12-17T05:18:59Z

btw npm is only part of node-win so it makes me wonder why there is npm for windows and not for other envs

ingorichter · 2015-12-17T07:31:01Z

That's a good question to ask. What happens if you don't download it? Are you able to build and run the tests?

jasonsanjose · 2015-12-17T16:42:45Z

Whew. That took some digging but there's the answer c913ec8. We used npm to install a dependency on ws for websocket support.

In #209 we moved that code from @joelrbrandt's git repo into the brackets-shell repo, but we never removed the npm dependency.

Looks safe to remove to me.

ingorichter · 2015-12-17T18:17:45Z

Great! Thanks for digging this up @jasonsanjose. @zaggino do want to remove it in this PR?

zaggino · 2015-12-17T20:38:10Z

Yes, because npm package is no longer on the nodejs server and the format of the tar in npm registry is a bit different.

ficristo · 2016-05-13T13:59:25Z

@zaggino @ingorichter Is this blocked on something?

abose · 2016-05-13T15:35:15Z

@ficristo Have you tested if

the existing file watching mechanism is properly woking with updated node engine?
installing extensions from the extension manager works.
Unit tests relating to extension pass
instant search is working
Unit tests relating to instant search is working.
Once all these are verified, I could run some test cases with updated node.

abose · 2016-05-13T15:38:59Z

Oh wait! it was @zaggino 's PR 😅 Wrongly addressed to @ficristo .

zaggino · 2016-05-16T23:52:09Z

This is not finished, I've run into fatal error in forked process and wasn't able to get around it. Compiling brackets works fine on my machine but not with this PR.

zaggino · 2016-05-17T01:34:32Z

if anyone wants to try to compile the shell with this PR, go ahead ... maybe something is wrong with my machine, I'll test this on Mac when I get a chance

ficristo · 2016-05-31T12:37:46Z

tasks/setup.js

@@ -377,22 +377,12 @@ module.exports = function (grunt) {

        var done        = this.async(),


Now this is unused: remove or call it.
(I've had some trouble testing the PR because of this)

ficristo · 2016-05-31T18:22:18Z

Some days ago I tryed this PR and had some problems.
I tryed again today and seems to run fine on Windows... 😕
(I've removed the done variable in my test)

ingorichter · 2016-06-07T00:10:45Z

I tried this today and it failed to download the node binary. The current download URL is http://nodejs.org/dist/v4.4.4/node-v4.4.4-darwin-x86.tar.gz, but this version is no longer available. Only the x64 version is available for OSX. So, this needs to be updated and then the question is: does it compile and work with the x64 version?

zaggino · 2016-07-30T09:22:28Z

I'd be great if someone could tackle this...
brackets-userland/brackets-eslint#36

AdriVanHoudt · 2016-08-02T07:21:22Z

what is blocking this?

ficristo · 2016-08-02T17:02:46Z

@AdriVanHoudt first things that come to my mind:

I left some comments that should be answered.
I think the version for OSX should be changed. See @ingorichter comment.
Verify all steps mentioned by @abose.
What we will do for Linux since is building from another branch?

ingorichter · 2016-08-02T17:58:29Z

I tried again last night and we have to change to the x64 variant. Changing it to x64 makes the download and build succeed. I haven't tested yet if Brackets launches and behaves properly (my machine had some thermal issues and shutdown down :-/). I can test it tonight on a different machine.

zaggino · 2016-08-03T00:02:24Z

I've changed Mac to the 64 bits; I've updated to the latest node (as there's little point to upgrading to v4 now that v6 is out); Removed some remaining npm references;

I'd love if someone from Adobe could try to build this on the machines that they use to build releases as my current Windows machine stopped being able to compile brackets-shell for whatever reason.

If I have a valid build of shell, I can start working on a PR of making https://github.com/adobe/brackets work with it.

PR for linux build branch should also not be an issue later.

zaggino · 2016-08-03T05:18:37Z

Guys, what about not porting fsevents_win and using https://github.com/paulmillr/chokidar for watching file system changes? If it's good for others, it should be good for Brackets, no? @ficristo @ingorichter

http://ourcodeworld.com/articles/read/160/watch-files-and-directories-with-electron-framework

ficristo · 2016-08-03T05:55:07Z

@zaggino I'm totally in favor: adobe/brackets#12190

zaggino · 2016-08-03T06:34:28Z

Ok, I'm gonna try building this shell and run tests using adobe/brackets#12190 branch, we'll see what happens. Anyone else is welcome to do the same :)

ingorichter · 2016-08-03T23:49:32Z

@zaggino I like the idea. Frees some resources for us. I'll give it a try tonight

zaggino · 2016-08-26T12:41:12Z

Now that adobe/brackets#12647 has landed in master, there're no other native dependencies that would block this from merging.

Please review/merge @ingorichter @ficristo

I'd love to see this land for 1.8 to solve brackets-userland/brackets-eslint#36 and also adobe/brackets#11748 (comment)

ficristo · 2016-08-28T12:47:59Z

To test this I've run only the test suite on Windows 10.
I see some consistent failures:

Integration \ FindInFiles
MainView \ MainViewManager
Extensions \ Code Folding
Extensions \ HealthData

but I doubt any of these is related to this PR.
For example I'm sure the FindInFiles is on master too, but I didn't test master to check the others.

ficristo · 2016-09-01T19:21:14Z

I tryed both on Windows10 and OSX and see some consistent failures on Integration \ FindInFiles and Extensions \ HealthData but I'm sure they are on master too.
@zaggino have you checked @abose comment?
Otherwise I think this is ready.

zaggino · 2016-09-01T23:30:08Z

@ficristo @abose

yes, file watching was updated to support node 6 and that's already in master
yes, installing extensions from the extension manager works.
yes, unit tests relating to extension manager pass
yes, instant search is working
yes, unit tests related to instant search are working

running this branch and latest master, no unit test is failing, from integration tests there are two failures which are definitely not related to this

nethip · 2016-09-02T04:47:48Z

@zaggino and everyone else on this thread. Thanks for your amazing efforts in taking up this herculean task. As always, I am in awe 👍

nethip · 2016-09-02T04:49:53Z

Gruntfile.js

@@ -71,7 +71,7 @@ module.exports = function (grunt) {
            },
            "node-mac": {
                "dest"      : "<%= downloads %>",
-                "src"       : "http://nodejs.org/dist/v<%= node.version %>/node-v<%= node.version %>-darwin-x86.tar.gz"
+                "src"       : "http://nodejs.org/dist/v<%= node.version %>/node-v<%= node.version %>-darwin-x64.tar.gz"


We might want to leave the mac node to be 32 bit, as our native node extensions are still 32 bit and they would not load on 64 bit node.

Which native node extensions are you talking about?

Node doesn't have 32-bit builds for Mac anymore:

Yeah. Now that we have moved to Chokidar, I don't think we will have any references to native node extensions. I was referring to this library which we were using earlier for file watching.

https://github.com/adobe/brackets/tree/release/src/filesystem/impls/appshell/node/node_modules/fsevents

I think then it should be fine to go with 64 bit.

zaggino mentioned this pull request Dec 18, 2015

Unifying third-party dependency management (through npm) adobe/brackets#12006

Open

ingorichter self-assigned this Jan 8, 2016

petetnt mentioned this pull request Feb 9, 2016

nodejs started from brackets not stable adobe/brackets#12202

Closed

zaggino force-pushed the node4 branch from b55d5a2 to ef2613d Compare May 16, 2016 23:56

ficristo reviewed May 31, 2016
View reviewed changes

zaggino added 4 commits August 3, 2016 09:59

update node and npm versions to latest LTS

5d1519c

remove npm

52133aa

fix incorrect reference

8e7ed7b

update to latest node

ec0e60c

zaggino force-pushed the node4 branch from 04dae74 to ec0e60c Compare August 2, 2016 23:59

zaggino mentioned this pull request Aug 3, 2016

porting fsevents_win to node 6.3.1 adobe/brackets#12642

Closed

put nodeWriteVersion back

652cec0

zaggino mentioned this pull request Aug 3, 2016

[PoC] Use chokidar instead of the fsevents/fsevents_win combo adobe/brackets#12190

Closed

zaggino added the high priority label Aug 3, 2016

zaggino changed the title ~~update node and npm versions to latest LTS~~ update shell node version to 6.3.1 Aug 3, 2016

zaggino mentioned this pull request Aug 3, 2016

Make file watching compatible with node version 6.x (runs on 0.10 too) adobe/brackets#12647

Merged

support downloading 64-bit nodejs

c2cb017

ficristo added this to the Release 1.8 milestone Aug 26, 2016

This was referenced Aug 26, 2016

Built-in nodejs version adobe/brackets#11748

Closed

node 6.3.1 for linux #574

Merged

nethip reviewed Sep 2, 2016
View reviewed changes

ficristo merged commit fe347d9 into master Sep 3, 2016

ficristo deleted the node4 branch September 3, 2016 06:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update shell node version to 6.3.1 #543

update shell node version to 6.3.1 #543

zaggino commented Dec 14, 2015

ingorichter commented Dec 17, 2015

zaggino commented Dec 17, 2015

ingorichter commented Dec 17, 2015

jasonsanjose commented Dec 17, 2015

ingorichter commented Dec 17, 2015

zaggino commented Dec 17, 2015

ficristo commented May 13, 2016

abose commented May 13, 2016

abose commented May 13, 2016

zaggino commented May 16, 2016

zaggino commented May 17, 2016

ficristo May 31, 2016

ficristo commented May 31, 2016 •

edited

Loading

ingorichter commented Jun 7, 2016

zaggino commented Jul 30, 2016

AdriVanHoudt commented Aug 2, 2016

ficristo commented Aug 2, 2016

ingorichter commented Aug 2, 2016

zaggino commented Aug 3, 2016 •

edited

Loading

zaggino commented Aug 3, 2016 •

edited

Loading

ficristo commented Aug 3, 2016

zaggino commented Aug 3, 2016

ingorichter commented Aug 3, 2016

zaggino commented Aug 26, 2016

ficristo commented Aug 28, 2016 •

edited

Loading

ficristo commented Sep 1, 2016

zaggino commented Sep 1, 2016

nethip commented Sep 2, 2016

nethip Sep 2, 2016

zaggino Sep 2, 2016

zaggino Sep 2, 2016

nethip Sep 2, 2016

		@@ -377,22 +377,12 @@ module.exports = function (grunt) {

		var done = this.async(),

update shell node version to 6.3.1 #543

update shell node version to 6.3.1 #543

Conversation

zaggino commented Dec 14, 2015

ingorichter commented Dec 17, 2015

zaggino commented Dec 17, 2015

ingorichter commented Dec 17, 2015

jasonsanjose commented Dec 17, 2015

ingorichter commented Dec 17, 2015

zaggino commented Dec 17, 2015

ficristo commented May 13, 2016

abose commented May 13, 2016

abose commented May 13, 2016

zaggino commented May 16, 2016

zaggino commented May 17, 2016

ficristo May 31, 2016

Choose a reason for hiding this comment

ficristo commented May 31, 2016 • edited Loading

ingorichter commented Jun 7, 2016

zaggino commented Jul 30, 2016

AdriVanHoudt commented Aug 2, 2016

ficristo commented Aug 2, 2016

ingorichter commented Aug 2, 2016

zaggino commented Aug 3, 2016 • edited Loading

zaggino commented Aug 3, 2016 • edited Loading

ficristo commented Aug 3, 2016

zaggino commented Aug 3, 2016

ingorichter commented Aug 3, 2016

zaggino commented Aug 26, 2016

ficristo commented Aug 28, 2016 • edited Loading

ficristo commented Sep 1, 2016

zaggino commented Sep 1, 2016

nethip commented Sep 2, 2016

nethip Sep 2, 2016

Choose a reason for hiding this comment

zaggino Sep 2, 2016

Choose a reason for hiding this comment

zaggino Sep 2, 2016

Choose a reason for hiding this comment

nethip Sep 2, 2016

Choose a reason for hiding this comment

ficristo commented May 31, 2016 •

edited

Loading

zaggino commented Aug 3, 2016 •

edited

Loading

zaggino commented Aug 3, 2016 •

edited

Loading

ficristo commented Aug 28, 2016 •

edited

Loading