Skip to content
This repository has been archived by the owner on Sep 2, 2021. It is now read-only.

Implement Node.js for Linux #306

Merged
merged 13 commits into from
Aug 22, 2013
Merged

Implement Node.js for Linux #306

merged 13 commits into from
Aug 22, 2013

Conversation

jasonsanjose
Copy link
Member

Based on an earlier pull request from @timburgess, #278. This also includes commits from #305 that fix the installer build. Finally, I've updated the gyp and grunt scripts to include node.

Original description:

This is some initial work in getting the node process up & running in Linux.

With this code, brackets-shell kicks off a mutexed launch thread that starts the Node executable in a subprocess.
As per the latest linux setup script, Node is launched from brackets-shell/deps/node/bin/node.

A file descriptor in the startup thread is piped to STDIN in the subprocess.
I have experimented with passing input to the node process to eval(). This works but Node appears to buffer it's output if it detects that STDIN is not a terminal. If I run node in interactive mode i.e. 'node -i' it stops buffering but produces a lot more output than I think is wanted.

I'd appreciate getting some feedback on how to run Node as then I can add code to setup a pipe from the subprocess STDOUT to a thread that reads the output.

// TODO linux preferences
//#define GROUP_NAME @""
//#define APP_NAME @"Brackets"
//#define WINDOW_TITLE APP_NAME
Copy link
Contributor

Choose a reason for hiding this comment

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

These defines are presently unused but will likely be used by GTK calls in the same way they are used on Windows & OS X. So in the meantime, I have simply commented them out.

@timburgess
Copy link
Contributor

+1 to merge. Grunt build produces a binary which runs for me OK and seems to have no issue with node-side extension code.

@jasonsanjose
Copy link
Member Author

Thanks @timburgess. We still need to wait for #305 to land. I'll merge this pull soon after.


// Determines whether the current node process has run long
// enough that a restart is warranted, and initiates the startup
// if so. Can also optionally terminate the running process.
void restartNode(bool terminateCurrentProcess) {

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll add a TODO here and add a task to the user story.

@JeffryBooher
Copy link
Contributor

I was able to build Brackets with this branch and the -3 is definitely gone! 👍

@jasonsanjose
Copy link
Member Author

Merging. Awesome work @timburgess! Also thanks for reviewing my gyp and grunt changes. Thanks @JeffryBooher for confirming the bug fix.

This. Is. Big. 🙌

jasonsanjose added a commit that referenced this pull request Aug 22, 2013
@jasonsanjose jasonsanjose merged commit 86da682 into master Aug 22, 2013
@jasonsanjose jasonsanjose deleted the timburgess/linux_nodejs branch August 22, 2013 05:35
@timburgess
Copy link
Contributor

No problem. Glad it is now in master and hopefully should kill a lot of the recent Linux functionality questions..

@timburgess
Copy link
Contributor

@jasonsanjose I'd value being listed in the Sprint 30 Release Notes as a contributor ;-)
https://github.com/adobe/brackets-shell/commits/master/appshell/appshell_node_process_linux.cpp

@jasonsanjose
Copy link
Member Author

Yikes! I guess I missed this one because I re-submitted the pull request. Sorry about that! I've updated the release notes https://github.com/adobe/brackets/wiki/Release-Notes:-Sprint-30.

@njx
Copy link
Contributor

njx commented Sep 6, 2013

FYI, if what happened is that you did a merge/squash and so lost the original commit, in future you can specify --author=<author> to git commit to set the original author. The <author> can be a portion of the author's name or email address.

@jasonsanjose
Copy link
Member Author

Nope, wasn't a squash. I think we normally just look at the pull request author vs. the commit author.

@njx
Copy link
Contributor

njx commented Sep 6, 2013

Ah. Maybe when reopening a pull request, we could put the original author in the title of the new one just so we'll notice it when scanning for the release notes.

@jasonsanjose
Copy link
Member Author

Good idea!

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.

4 participants