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 use -latomic on macOS #30099

Closed
wants to merge 1 commit into from
Closed

Don't use -latomic on macOS #30099

wants to merge 1 commit into from

Conversation

ryandesign
Copy link
Contributor

Fixes build when using open-source clang. (Builds using Apple clang from Xcode
7 and later were already working, due to the fact that node's build system
misidentifies their llvm_version as "0.0" and thus the flag wasn't being added.)

Closes #30093

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

Fixes build when using open-source clang. (Builds using Apple clang from Xcode
7 and later were already working, due to the fact that node's build system
misidentifies their llvm_version as "0.0" and thus the flag wasn't being added.)

Closes #30093
@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Oct 23, 2019
Copy link
Member

@richardlau richardlau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks okay to me but I don't have a mac so can't test this.

cc @nodejs/platform-macos @nodejs/build-files

@nodejs-github-bot
Copy link
Collaborator

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 25, 2019
@BridgeAR
Copy link
Member

Ping @nodejs/platform-macos @nodejs/build-files

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 26, 2019

@BridgeAR BridgeAR requested a review from devsnek January 1, 2020 17:42
@devsnek devsnek removed their request for review January 1, 2020 17:57
@devsnek
Copy link
Member

devsnek commented Jan 1, 2020

I don't have a mac anymore, so I can't test or verify this.

@ljharb
Copy link
Member

ljharb commented Jan 1, 2020

./configure && make passed on my Mac with an unset CXX, and CXX=g++. I'll leave it to someone else to test homebrew's clang++.

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RSLGTM

BridgeAR pushed a commit that referenced this pull request Jan 2, 2020
Fixes build when using open-source clang.

Builds using Apple clang from Xcode v7 and later were already working,
due to the fact that node's build system misidentifies their
llvm_version as "0.0" and thus the flag wasn't being added.

PR-URL: #30099
Fixes: #30093
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@BridgeAR
Copy link
Member

BridgeAR commented Jan 2, 2020

Landed in 4bd0d8c 🎉

@BridgeAR BridgeAR closed this Jan 2, 2020
@BridgeAR
Copy link
Member

BridgeAR commented Jan 2, 2020

@ryandesign congratulations on your first commit to Node.js! 🎉

@ryandesign ryandesign deleted the patch-1 branch January 3, 2020 00:28
BridgeAR pushed a commit that referenced this pull request Jan 3, 2020
Fixes build when using open-source clang.

Builds using Apple clang from Xcode v7 and later were already working,
due to the fact that node's build system misidentifies their
llvm_version as "0.0" and thus the flag wasn't being added.

PR-URL: #30099
Fixes: #30093
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@BridgeAR BridgeAR mentioned this pull request Jan 7, 2020
targos pushed a commit that referenced this pull request Jan 14, 2020
Fixes build when using open-source clang.

Builds using Apple clang from Xcode v7 and later were already working,
due to the fact that node's build system misidentifies their
llvm_version as "0.0" and thus the flag wasn't being added.

PR-URL: #30099
Fixes: #30093
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
Fixes build when using open-source clang.

Builds using Apple clang from Xcode v7 and later were already working,
due to the fact that node's build system misidentifies their
llvm_version as "0.0" and thus the flag wasn't being added.

PR-URL: #30099
Fixes: #30093
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
liu-kan added a commit to liu-kan/pynng that referenced this pull request Aug 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong condition for deciding when to add -latomic
6 participants