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

change the min mac os x version required for `node-gyp rebuild` #23685

Conversation

Projects
None yet
8 participants
@alexventuraio
Copy link

commented Oct 16, 2018

It fixes an issue when running npm install for a project with Node v6.11.5 and npm v3.10.10 on Mac OS X 10.4 Mojave and Xcode v9.4.1 || Xcode v10.0. It asks for the 'utility' file when running node-gyp rebuild.

Fixes: nodejs/node-gyp#1574

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

This solution was based on this fix from @yerassyl94.

change the min mac os x version required.
It fixes an issue when running `npm install` for a project with *Node
v6.11.5* and *npm v3.10.10* on *Mac OS X 10.4 Mojave* and *Xcode v9.4.1
|| Xcode v10.0*. It asks for the 'utility' file when running
`node-gyp rebuild`.

Fixes: nodejs/node-gyp#1574

@refack refack force-pushed the alexventuraio:utility-file-not-found-node-gyp-rebuild branch from a80901c to bca6c16 Oct 16, 2018

@refack

This comment has been minimized.

Copy link
Member

commented Oct 16, 2018

Rebased on v6.x

@refack

refack approved these changes Oct 16, 2018

@refack

This comment has been minimized.

Copy link
Member

commented Oct 16, 2018

@nodejs/lts This fixes issues with native addons.
macOS (which the majority of use is by far non server) 10.7 is de-facto EOL, with last update 10.7.5 at September 19, 2012. (AFAIK Apple doesn't have an explicit EOL policy)

@gdams gdams added the semver-major label Oct 16, 2018

@refack

This comment has been minimized.

Copy link
Member

commented Oct 16, 2018

/CC @nodejs/platform-macos @nodejs/node-gyp @nodejs/addon-api

@gdams the point is to land this as semver-patch.

@richardlau
Copy link
Member

left a comment

For Node.js 6 PR's should target v6.x-staging instead of v6.x.

On a technical level, bumping MACOSX_DEPLOYMENT_TARGET is semver-major and therefore I'm -1 on this for any currently supported releases (including 6). An argument could be made to bump it for Node.js 11 to 10.11 to match 9453240#diff-396d0b75211f5646b2fa730fbdf80a40
cc @nodejs/platform-macos

Addons can always override MACOSX_DEPLOYMENT_TARGET in their own binding.gyp files. Neither the deployment target nor the v8.h file referenced in nodejs/node-gyp#1574 have been changed in years. Is it possible the issue is due to Mojave/Xcode?

@refack

This comment has been minimized.

Copy link
Member

commented Oct 16, 2018

On a technical level, bumping MACOSX_DEPLOYMENT_TARGET is semver-major

We have a blanket policy that we can drop support for EOL platforms mid stream.
This patch is to fix an extant issue with building native addons with node6 on macOS, so we might consider doing something special, but it's up for discussion 🤷‍♂️

@refack refack changed the base branch from v6.x to v6.x-staging Oct 16, 2018

@yerassyl94

This comment has been minimized.

Copy link

commented Oct 16, 2018

Xcode has been changed its C++ standar library from "libstdc++ (GNU C++ standard library)" to "libc++ (LLVM C++ standard library with C++11 support)".
This issue popped when I have updated my Mac to Mojave

@refack

This comment has been minimized.

Copy link
Member

commented Oct 16, 2018

ping @ljharb?

@ljharb

This comment has been minimized.

Copy link
Contributor

commented Oct 16, 2018

EOL or not (and as you said, Apple has no such policy, which makes it not EOL, which means this blanket policy doesn’t technically apply), it will likely break a lot of Mac users if this ships - hopefully it can wait for a semver major so it’s both a cleaner break, and so i can update nvm in advance of a release to be able to handle it.

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented Oct 16, 2018

Bumping MACOSX_DEPLOYMENT_TARGET mid-cycle can end up breaking things. It's not something we can do without knowing what exactly it turns on or off.

An example from 2016: 204f3a8 bumped to 10.7, turned on PIE (position-independent executable), and broke the profiler on OS X because it didn't know about PIE slides.

For add-ons, there is a workaround: bnoordhuis/node-heapdump#120 (comment)

@refack

This comment has been minimized.

Copy link
Member

commented Oct 16, 2018

@AlexVentura I'm going to close this. It seems like the stage in which we are in the life cycles of node6 isn't a good time to bump this variable.

FTR, possible workaround:

For users:

env CXXFLAGS="-mmacosx-version-min=10.9" npm install

For addon authors:

add this to your bindings.gyp (but as mentioned above test)

  'xcode_settings': {
    'MACOSX_DEPLOYMENT_TARGET': '10.9',
  },

@refack refack closed this Oct 16, 2018

@alexventuraio

This comment has been minimized.

Copy link
Author

commented Oct 16, 2018

@refack Got it! Thx to everyone for clarifying the topic a bit more.

@richardlau

This comment has been minimized.

Copy link
Member

commented Oct 17, 2018

Xcode has been changed its C++ standar library from "libstdc++ (GNU C++ standard library)" to "libc++ (LLVM C++ standard library with C++11 support)".
This issue popped when I have updated my Mac to Mojave

This sounds familiar. We ran into libstdc++ vs libc++ issues on macOS for node-report (nodejs/node-report#22, nodejs/node-report#56). See nodejs/node-gyp#469 (comment) for why it might not be a good idea to change the MACOSX_DEPLOYMENT_TARGET for addons for Node.js 6 if you are also not compiling Node.js from source (addons will end up linking against the other C++ library).

Node.js 8 and above specifies libc++:

'CLANG_CXX_LIBRARY': 'libc++',
, overriding the default for the deployment target.

For add-ons, there is a workaround: bnoordhuis/node-heapdump#120 (comment)

Thanks. From that issue and the Xcode 10 release notes: https://developer.apple.com/documentation/xcode_release_notes/xcode_10_release_notes

Building with libstdc++ was deprecated with Xcode 8 and is not supported in Xcode 10 when targeting iOS. C++ projects must now migrate to libc++ and are recommended to set a deployment target of macOS 10.9 or later, or iOS 7 or later. Besides changing the C++ Standard Library build setting, developers should audit hard-coded linker flags and target dependencies to remove references to libstdc++ (including -lstdc++, -lstdc++.6.0.9, libstdc++.6.0.9.tbd, and libstdc++.6.0.9.dylib). Project dependencies such as static archives that were built against libstdc++ will also need to be rebuilt against libc++. (40885260)

reconbot added a commit to serialport/node-serialport that referenced this pull request Dec 30, 2018

fix: prebuild on mojave
OSX Mojave has issues building for node 6. Prebuild tries to build for everything we support and that includes node 6. Node 6 was built with `libstdc++` and osx has (over the past few years, along with nodejs core) moved to `libc++` as the standard library. It no longer supports it with xcode 10 and that's what ships with Mojave.

References
- Workaround until we drop node 6 nodejs/node#23685 (comment)
- More background nodejs/node#24648

I reformatted trying to work around this issue. Glad it's solved.

@reconbot reconbot referenced this pull request Dec 30, 2018

Merged

fix: prebuild on mojave #1759

reconbot added a commit to serialport/node-serialport that referenced this pull request Dec 31, 2018

fix: prebuild on mojave (#1759)
OSX Mojave has issues building for node 6. Prebuild tries to build for everything we support and that includes node 6. Node 6 was built with `libstdc++` and osx has (over the past few years, along with nodejs core) moved to `libc++` as the standard library. It no longer supports it with xcode 10 and that's what ships with Mojave.

References
- Workaround until we drop node 6 nodejs/node#23685 (comment)
- More background nodejs/node#24648

I reformatted trying to work around this issue. Glad it's solved.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.