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

Fix configure script to work with Apple Clang 10 #21200

Closed
wants to merge 1 commit into from

Conversation

saagarjha
Copy link
Contributor

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

This is a simple fix to the configure script so it doesn't spuriously drop out for newer versions of Apple's Clang:

$ clang --version
Apple LLVM version 10.0.0 (clang-1000.10.25.5)
Target: x86_64-apple-darwin18.0.0
Thread model: posix
InstalledDir: /Applications/Xcode-beta.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin

Please do let me know if there's something wrong with my implementation; I haven't tested this on systems other than macOS so it might have silently broke something there.

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Jun 7, 2018
@apapirovski
Copy link
Member

apapirovski commented Jun 7, 2018

Hi @saagarjha, thanks for taking this on. It appears there's already an earlier PR implementing a similar change: #21183

@saagarjha
Copy link
Contributor Author

Sorry, my bad. I should have searched before making the pull request rather than checking when I had the issue yesterday. However, I did look through that pull request, it seems like my code has an additional patch that @mistydemeo's branch doesn't in get_xcode_version. Without it, the configure script halts:

$ ./configure
ERROR: Did not find a new enough assembler, install one or build with
       --openssl-no-asm.
       Please refer to BUILDING.md

@apapirovski
Copy link
Member

@saagarjha See #21173 for that.

@saagarjha
Copy link
Contributor Author

Ah, got it. In that case I guess I'll close this pull request and let you guys merge those instead. Though, might I suggest using Python's built-in tuple comparison operator rather than rolling our own?

@saagarjha saagarjha closed this Jun 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants