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

Don't include common.gypi from node source tree #1118

Open
bnoordhuis opened this issue Feb 15, 2017 · 6 comments
Open

Don't include common.gypi from node source tree #1118

bnoordhuis opened this issue Feb 15, 2017 · 6 comments

Comments

@bnoordhuis
Copy link
Member

node-gyp includes $nodedir/common.gypi when creating build files for an add-on.

Per nodejs/CTC#62 (comment), that probably isn't future-proof and it's a bad idea in general: it makes add-ons pick up compiler and linker settings that aren't intended for them and may be actively harmful. People have filed bug reports about it.

Plan of action:

  1. Merge common.gypi into addon.gypi.
  2. Strip everything that isn't needed by add-ons.
  3. Test the bejebus out of it with citgm.
@murgatroid99
Copy link

I would support this change. I got burned by the fact that common.gypi explicitly disables -Werror

@richardlau
Copy link
Member

Node.js 10's common.gypi requires C++14:
https://github.com/nodejs/node/blob/cf41627411886000429bde058a6594fb7f6d6d47/common.gypi#L324

which may expose anyone using older compilers to compile addons.

@bnoordhuis
Copy link
Member Author

Yep, but that flows on down from V8. We'd still carry that change with a separate common.gypi.

@rvagg
Copy link
Member

rvagg commented Jun 20, 2019

still planning on making this happen @bnoordhuis? I fear this is just going to linger as a perpetually open issue unless you make it happen, I suspect there's too many dragons for most mortals to want to attempt this

@bnoordhuis
Copy link
Member Author

@rvagg It's one of those projects that I do want to get around to someday but I don't know when. If you want to close it, that's fine by me.

Good job on the cleanup action, by the way. :-)

@rvagg
Copy link
Member

rvagg commented Jun 20, 2019

I think I got throttled along the way with my cleanup, I stopped being able to close issues, a red error box from GitHub as if I was blocked. Seems to be working again now.

I don't mind this being left open if it's genuinely a TODO, your call. I'm just aiming to get rid of the clutter so this starts being manageable again.

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

No branches or pull requests

4 participants