Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

node-waf on darwin produces i386 binaries when x86_64 expected #3633

Closed
dimsmol opened this issue Jul 4, 2012 · 7 comments
Closed

node-waf on darwin produces i386 binaries when x86_64 expected #3633

dimsmol opened this issue Jul 4, 2012 · 7 comments

Comments

@dimsmol
Copy link

dimsmol commented Jul 4, 2012

Node 0.8 works in 64-bit mode on darwin, but node-waf is forced to produce 32-bit binaries.
For 64-bit platform this makes unusable a lot of modules which are still using waf.

The problem cannot be fixed by providing environment variables due to issue #3524
One can obtain universal binaries as a result of compilation, but not linking (funny, yeah).

Erroneous code is in the node_addon.py:

  ## On Mac OSX we need to use mac bundles
  if Options.platform == 'darwin':
    if 'i386' in Utils.cmd_output(['file', nodebin]):
      conf.env.append_value('CPPFLAGS_NODE', ['-arch', 'i386'])
      conf.env.append_value('CXXFLAGS_NODE', ['-arch', 'i386'])
      conf.env.append_value('LINKFLAGS', ['-arch', 'i386'])
      conf.env['DEST_CPU'] = 'i386'
    conf.check_tool('osx')

While node 0.8 is compiled as universal binary, this code forces waf to use i386 only.

As a workaround, you can remove 5 lines of code starting from if 'i386' in Utils.cmd_output(['file', nodebin]): and node-waf will create x86_64 binaries as expected by platform, but it possibly will brake things up for 32-bit platforms.

@dimsmol
Copy link
Author

dimsmol commented Jul 4, 2012

Probably something like this will resolve the issue correctly:

    if 'i386' in Utils.cmd_output(['file', nodebin]):
      conf.env.append_value('CPPFLAGS_NODE', ['-arch', 'i386'])
      conf.env.append_value('CXXFLAGS_NODE', ['-arch', 'i386'])
      conf.env.append_value('LINKFLAGS', ['-arch', 'i386'])
    if 'x86_64' in Utils.cmd_output(['file', nodebin]):
      conf.env.append_value('CPPFLAGS_NODE', ['-arch', 'x86_64'])
      conf.env.append_value('CXXFLAGS_NODE', ['-arch', 'x86_64'])
      conf.env.append_value('LINKFLAGS', ['-arch', 'x86_64'])

But still unsure what to place instead of conf.env['DEST_CPU'] = 'i386'

@TooTallNate
Copy link

Yes, this a problem with the .pkg installer on OS X. If you install from source the problem goes away.

I don't think your proposed patch will work though, since running file node on the universal binary prints out 2 lines, so both if cases will be true (which I guess will result in a universal binary, but I'm not sure that's what we really want).

Perhaps try parsing the output of node -pe process.arch.

@Sannis
Copy link

Sannis commented Jul 5, 2012

@dimsmol node 0.8 is a good time to use bindings.gyp for binary addons :)

@ryandesign
Copy link

@TooTallNate it's not problem with the .pkg installer. On the contrary: it's a feature that the installer now installs universal binaries. Users requested this. The request was implemented in nodejs 0.7.5. The problem is not due to installing from a binary vs. installing from source; the problem is due to installing universal vs. installing for a single architecture. Installing software universal is the normal and expected situation on OS X, but not all of nodejs appears to be ready for it. That is the bug.

node_addon.py assumes a file can be built for only a single architecture. This is incorrect.

Yes, certainly both "if" cases in the solution proposed by @dimsmol will activate. That was clearly his intention, so that if node is built universal, then modules will be built universal as well. Sounds good to me.

I don't know what DEST_CPU is, and it sounds like another case of node assuming there is only one CPU type being built for, an assumption which should be corrected.

Side note: relying on the "file" command to tell you what architectures a file is built for will fail when a file is built for more than one architecture and you have installed a non-Apple version of the "file" command, such as the one you can download from http://www.darwinsys.com/file/ or using "sudo port install file" using MacPorts. Better to parse the output of the "lipo -info" command instead which should always work.

@TooTallNate
Copy link

@ryandesign

it's not problem with the .pkg installer.

No, not a problem with the installer, or the fact that it's a universal binary, but the problem is with how node-waf is trying to detect the arch, which does't match up to process.arch when you invoke node.

On the contrary: it's a feature that the installer now installs universal binaries. Users requested this.

But of course; I implemented it ;) (e60b18b).

Installing software universal is the normal and expected situation on OS X, but not all of nodejs appears to be ready for it. That is the bug.

node_addon.py assumes a file can be built for only a single architecture. This is incorrect.

Yes, certainly both "if" cases in the solution proposed by @dimsmol will activate. That was clearly his intention, so that if node is built universal, then modules will be built universal as well. Sounds good to me.

I can see how this could go either way. The reason I was thinking add ons should only be built for a single architecture is because usually when you link to external libraries (i.e. installed via Homebrew), they get compiled with only a single architecture:

$ file /usr/local/lib/libzmq.dylib 
/usr/local/lib/libzmq.dylib: Mach-O 64-bit dynamically linked shared library x86_64

Thus, attempting to build a universal addon would fail during linking for the 32-bit part. If there's any support for universal addons (which has minimal value IMO) then it should be opt-in only.

@bjo3rn
Copy link

bjo3rn commented Jul 9, 2012

@dimsmol try conf.env['DEST_CPU'] = 'x64' inside the if 'x86_64' ... clause. Worked for me (for a different library):

$ file libxmljs.node libxmljs.node: Mach-O universal binary with 2 architectures libxmljs.node (for architecture i386): Mach-O bundle i386 libxmljs.node (for architecture x86_64): Mach-O 64-bit bundle x86_64

@bnoordhuis
Copy link
Member

We're not going to fix this. Bug module authors to upgrade to node-gyp. Closing.

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

No branches or pull requests

6 participants