-
-
Notifications
You must be signed in to change notification settings - Fork 210
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
Remove cross-compilation arch flags for MacOS #622
Conversation
`CMAKE_OSX_ARCHITECTURES` is already set in `build.js`, so there's no need to set it in `binding.gyp`. In fact, if the architecture is inferred from the host (and not via the `$ARCH` env var), this previously resulted in incorrectly cross-compiling.
'OTHER_CFLAGS': [ | ||
"<!(echo \"-arch ${ARCH:=x86_64}\")", | ||
], | ||
'OTHER_LDFLAGS': [ | ||
"<!(echo \"-arch ${ARCH:=x86_64}\")", | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The gyp flags are used for ZeroMQ, and build.js is used for libzmq (the upstream library used by ZeroMQ). There's a subtle difference between these two that's sometimes confusion for new contributors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
definitely confusing to me! Where does the ARCH
value here supposedly come from? I was finding it to be blank (on my ARM mac)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ARCH is defined in the CI or as an env variable in build.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If ARCH
is always defined, there should be no need for "<!(echo \"-arch ${ARCH:=x86_64}\")",
. It seems odd too that we should have to override the flags instead of simply omitting them.
I'll create an issue for what I thought was a symptom of this.
Submitted as issue #632. I'm not sure why you closed this PR (not a problem? wrong fix?) - and I would have appreciated a comment to that effect. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we now have access to native Arm machines, having these cross-compilation flags is not necessary. We can add them back via generic flags that can be passed to the compiler.
In fact, if the architecture is inferred from the host (and not via theCMAKE_OSX_ARCHITECTURES
is already set inbuild.js
, so there's no need to set it inbinding.gyp
.$ARCH
env var), this previously resulted in incorrect cross-compiling.Fixes #632