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

build: remove --xcode configure switch #20328

Closed
wants to merge 1 commit into from

Conversation

bnoordhuis
Copy link
Member

./configure --xcode ostensibly let you built with the Xcode IDE but
it has never been tested regularly since its introduction in 2012 and
probably has been broken for years. Remove it.

Fixes: #20324

`./configure --xcode` ostensibly let you built with the Xcode IDE but
it has never been tested regularly since its introduction in 2012 and
probably has been broken for years.  Remove it.

Fixes: nodejs#20324
@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Apr 26, 2018
@benjamingr
Copy link
Member

@bnoordhuis so, I just tested this locally and building with xcode almost works fine fine (though it takes a lot more time than regular make -j4). It doesn't look like it's too hard to fix - that said - I've never even realized this was a feature and never used it.

Pinging @nodejs/platform-macos to see if anyone has a strong opinion about removing it since it currently works (at least for me).

@bnoordhuis
Copy link
Member Author

For background, the --xcode switch depends on gyp's xcode generator. That generator never got much love even when gyp was still maintained upstream, let alone now. The most recent bug fix is from 3 or 4 years ago.

@targos
Copy link
Member

targos commented Apr 26, 2018

@targos targos added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 26, 2018
@joyeecheung
Copy link
Member

I have used it before and it worked just fine. Have not used it for quite a while but I don’t see a reason removing it? If we don’t want bug reports, emitting a warning about it being not supported should be fine. Whoever needs it can figure it out or fix the bug themselves, much like how we treat the android build files.

Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

Making my -1 explicit

@bnoordhuis
Copy link
Member Author

You'll still be able to select the xcode generator with ./configure -- -f xcode. That's how you select the eclipse generator, for example, which is in a similar state of neglect.

Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

LGTM since /configure -- -f xcode works

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Apr 29, 2018
`./configure --xcode` ostensibly let you built with the Xcode IDE but
it has never been tested regularly since its introduction in 2012 and
probably has been broken for years.  Remove it.

PR-URL: nodejs#20328
Fixes: nodejs#20324
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
@BridgeAR
Copy link
Member

Landed in a9051bb 🎉

@BridgeAR BridgeAR closed this Apr 29, 2018
MylesBorins pushed a commit that referenced this pull request May 4, 2018
`./configure --xcode` ostensibly let you built with the Xcode IDE but
it has never been tested regularly since its introduction in 2012 and
probably has been broken for years.  Remove it.

PR-URL: #20328
Fixes: #20324
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
@MylesBorins MylesBorins mentioned this pull request May 8, 2018
@thlorenz
Copy link
Contributor

Just running into this now and can confirm that building via Xcode is broken on current master.
However I used this feature heavily, i.e. to debug async-hooks while we were implementing them.
Without Xcode I would have taken a much longer time to understand how things work and discover the reasons for bugs that we encountered.

I just confirmed that things still build fine on the v8.x branch, so unlike stated, this actually did work until recently. Thus this is a regression that got fixed by removing the flag that made it easily accessible.

I understand that lldb exists as an option, but in a lot of cases the Xcode IDE makes it a lot easier to navigate code, inspect variables, etc. especially if debugging Node.js features and v8 API integration.
Obviously it's importance is only clear to us who are actually using Xcode.

I hope this will get fixed properly even if Xcode users seem to be in the minority :)

To be clear, removing the flag is not the problem, but releasing a new version with this regression doesn't seem to be the right thing to do IMHO and should be remedied.

@MylesBorins
Copy link
Contributor

@thlorenz do you have any time to dig in and see if you can fix building with xcode?

@thlorenz
Copy link
Contributor

@MylesBorins not ATM, but if this doesn't get fixed until I do I can have a look.
Parts of the breakage seems to be related to OpenSSL upgrade, so fixing could be a bit complex ;)

@joyeecheung
Copy link
Member

joyeecheung commented May 16, 2018

@thlorenz By the way if you want to use a GUI frontend for lldb debugging (that's basically what the Xcode debugger does), vscode-lldb works pretty well (although I have not found a way to bring the lldb command line prompt out so I cannot use llnode in vscode like what I used to do in Xcode).

EDIT: oh llnode in vscode actually works in the debug console. I guess I just have not used it enough to figure that out..

@rvagg
Copy link
Member

rvagg commented May 17, 2018

I had a bit of a play with this because I figured the openssl failures shouldn't be hard to fix since they were only recently introduced, and yes, they were easy to fix (once I figured out what the problem was). Unfortunately there's still a node_js2c shell script error and a v8_inspector_compress_protocol_json shell script error, probably introduced not too long ago but not noticed by the core team because nobody around here uses xcode for core.

I doubt this is too hard to fix, it might just be a matter of fixing paths in a gyp file or something, but I don't have time to figure these next steps out right now so I'm going to have to leave this on the floor for now. If anyone here, @thlorenz or others, want to tackle this, here's my branch with the openssl fixes and a revert of this commit: https://github.com/rvagg/io.js/tree/rvagg/reinstate-xcode-support

@zhuyingda
Copy link

Hi @thlorenz do you find any workaround to debug nodejs by using XCode IDE so far?
I have same problem too, and I am just the minority Xcode user you have mentioned. >.<

@thlorenz
Copy link
Contributor

No, but I haven't tried either so I cannot be of much help at this point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build in Xcode got Error on MacOS