Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix macOS electron 3.x compiles #339

Closed
wants to merge 1 commit into from
Closed

fix macOS electron 3.x compiles #339

wants to merge 1 commit into from

Conversation

the-j0k3r
Copy link
Contributor

@the-j0k3r the-j0k3r commented Aug 3, 2019

This was broken in 5db10ca made in #322

Consequence macOS builds for electron 3.x fail.

Error logs oof failure https://travis-ci.org/oznu/node-pty-prebuilt-multiarch/jobs/567184687

Related PR where the issue was first noticed homebridge#2

Progress on issue comment homebridge#2 (comment)

Relevant part of log with error

  CXX(target) Release/obj.target/pty/src/unix/pty.o
../src/unix/pty.cc:635:10: error: use of undeclared identifier 'openpty'
  return openpty(amaster, aslave, name, (termios *)termp, (winsize *)winp);
         ^
../src/unix/pty.cc:684:10: error: use of undeclared identifier 'forkpty'
  return forkpty(amaster, name, (termios *)termp, (winsize *)winp);
         ^
2 errors generated.

With this fix applied compiles are successful once again https://travis-ci.org/the-j0k3r/node-pty-prebuilt-multiarch/jobs/567269847

Notes: Chose node 8.0.0 because node-pty is building from node 8.0.0 up so 8.x is minimum version.

node_8_x:

@the-j0k3r
Copy link
Contributor Author

@Tyriar sorry for shameless ping, but really need this to work upstream.

src/unix/pty.cc Outdated Show resolved Hide resolved
@Tyriar Tyriar requested a review from deepak1556 August 8, 2019 02:16
Error logs 
https://travis-ci.org/oznu/node-pty-prebuilt-multiarch/jobs/567184687

```log
  CXX(target) Release/obj.target/pty/src/unix/pty.o
../src/unix/pty.cc:635:10: error: use of undeclared identifier 'openpty'
  return openpty(amaster, aslave, name, (termios *)termp, (winsize 
*)winp);
         ^
../src/unix/pty.cc:684:10: error: use of undeclared identifier 'forkpty'
  return forkpty(amaster, name, (termios *)termp, (winsize *)winp);
         ^
2 errors generated.
```
@deepak1556
Copy link
Contributor

deepak1556 commented Aug 8, 2019

Its a bug in electron for shipping all node src headers instead of the necessary header. Electron 3 is no longer a supported release line so fixing there is not an option. Also this is fixed in Electron 4 and higher. So I would say the apps should update from electron 3 for the best path forward. I am hesitant to take this path hack since its not needed for the long term.

@the-j0k3r
Copy link
Contributor Author

@deepak1556 Problem is Atom stable 1.39.1 uses electron 3.x and it wont be months before 1.41.0 is shipped and until then we have no working solution fpr the project.

Ill see if the node-pty-prebuilt-multiarch maintainer is willing to carry this patch until then, else Ive no idea.

Thanks either way.

@the-j0k3r
Copy link
Contributor Author

We are going to deal with this another way, We are at the mercy of Atom current production release electron 3.x version until this changes.

Thanks for the consideration. Will close this also =)

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

Successfully merging this pull request may close these issues.

3 participants