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

Remove cross-compilation arch flags for MacOS #622

Merged
merged 1 commit into from
Jun 18, 2024
Merged

Conversation

rotu
Copy link
Contributor

@rotu rotu commented Jun 15, 2024

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 incorrect cross-compiling.

Fixes #632

`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.
@rotu rotu marked this pull request as ready for review June 15, 2024 04:08
Comment on lines -137 to -142
'OTHER_CFLAGS': [
"<!(echo \"-arch ${ARCH:=x86_64}\")",
],
'OTHER_LDFLAGS': [
"<!(echo \"-arch ${ARCH:=x86_64}\")",
]
Copy link
Member

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.

Copy link
Contributor Author

@rotu rotu Jun 15, 2024

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)

Copy link
Member

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

Copy link
Contributor Author

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.

@rotu rotu marked this pull request as draft June 15, 2024 07:34
@aminya aminya closed this Jun 18, 2024
@rotu
Copy link
Contributor Author

rotu commented Jun 18, 2024

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.

@aminya
Copy link
Member

aminya commented Jun 18, 2024

#632 (comment)

@aminya aminya reopened this Jun 18, 2024
@aminya aminya changed the title Don't redundantly redefine macOS architecture Remove cross-compilation arch flags for MacOS Jun 18, 2024
@aminya aminya marked this pull request as ready for review June 18, 2024 20:16
Copy link
Member

@aminya aminya left a 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.

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

Successfully merging this pull request may close these issues.

Wrong architecture linker warnings on ARM64 macOS
2 participants