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

build: correctly detect clang version #5553

Closed
wants to merge 2 commits into from

Conversation

stefanmb
Copy link
Contributor

@stefanmb stefanmb commented Mar 3, 2016

Pull Request check-list

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)? Yes
  • Is the commit message formatted according to CONTRIBUTING.md? Yes
  • If this change fixes a bug (or a performance problem), is a regression
    test (or a benchmark) included? n/a
  • Is a documentation update included (if this change modifies
    existing APIs, or introduces new ones)? n/a

Affected core subsystem(s)

build

Description of change

We provide our own build system on top of OpenSSL's, part of which is responsible for detecting support for newer assembly instructions (AVX2, etc). The optimized assembly is located in the deps/openssl/asm folder, whereas the fallback path is in deps/openssl/asm_obsolete.

The configuration script relies on scraping "cc -v" to obtain the LLVM version number on Mac OS X. However, as of Xcode 7 this information is no longer included in the banner, see here for a history of the banners: https://gist.github.com/yamaya/2924292

As a result, on systems with Xcode 7 OpenSSL will be compiled with the obsolete assembly path, regardless of actual hardware capability. I've resolved this issue by separately checking for the Xcode version. Note that there doesn't seem to be a way of obtaining the LLVM version, other than manually encoding it in a table such as this https://en.wikipedia.org/wiki/Xcode#Version_comparison_table. Storing this information is not reasonable for the config script, so verifying the Xcode version is a better way.

Note that both the get_llvm_version and get_xcode_version functions rely on minimum versions to return a match. Please see here for more details: #5550 (comment)

I've tested this change on Ubuntu 14.04 and Mac OS 10.11.3.

Use the "Apple LLVM" version number since the banner has changed in
newer versions of Mac OS X, resulting in the obsolete assembler path
being used to compile OpenSSL.
@mscdex mscdex added the build Issues and PRs related to build files or the CI. label Mar 3, 2016
@jbergstroem
Copy link
Member

I install clang through homebrew -- think you have to potentially run both checks on darwin?

@stefanmb
Copy link
Contributor Author

stefanmb commented Mar 4, 2016

@jbergstroem I didn't replace the original check (which would catch clang), instead I just added an "or" checking for Xcode after it, in your case I believe it will short circuit after the first check (for "clang version").

@jbergstroem
Copy link
Member

just skimmed it; python and indentation :)

@@ -454,7 +454,7 @@ def get_llvm_version(cc):
'''
sys.exit()

match = re.search(r"(^clang version|based on LLVM) ([3-9]\.[0-9]+)",
match = re.search(regexp,
proc.communicate()[1])
Copy link
Member

Choose a reason for hiding this comment

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

This fits on one line now.

@bnoordhuis
Copy link
Member

LGTM with style nits. CI: https://ci.nodejs.org/job/node-test-pull-request/1847/

@stefanmb
Copy link
Contributor Author

stefanmb commented Mar 4, 2016

@bnoordhuis Thanks. Updated to reflect your comments, I've tried to follow https://www.python.org/dev/peps/pep-0008/#indentation with respect to breaking the argument list.

@bnoordhuis
Copy link
Member

Thanks Stefan, landed in 1792470.

@bnoordhuis bnoordhuis closed this Mar 4, 2016
bnoordhuis pushed a commit that referenced this pull request Mar 4, 2016
Use the "Apple LLVM" version number since the banner has changed in
newer versions of Mac OS X, resulting in the obsolete assembler path
being used to compile OpenSSL.

PR-URL: #5553
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@jasnell
Copy link
Member

jasnell commented Mar 7, 2016

Does this need to land on v4.x also?

@bnoordhuis
Copy link
Member

Looks like it. I've added the tag.

@stefanmb
Copy link
Contributor Author

stefanmb commented Mar 7, 2016

@jasnell @bnoordhuis Yep, it should, thanks!

@Fishrock123 Fishrock123 mentioned this pull request Mar 7, 2016
Fishrock123 pushed a commit that referenced this pull request Mar 8, 2016
Use the "Apple LLVM" version number since the banner has changed in
newer versions of Mac OS X, resulting in the obsolete assembler path
being used to compile OpenSSL.

PR-URL: #5553
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Fishrock123 pushed a commit that referenced this pull request Mar 8, 2016
Use the "Apple LLVM" version number since the banner has changed in
newer versions of Mac OS X, resulting in the obsolete assembler path
being used to compile OpenSSL.

PR-URL: #5553
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Mar 17, 2016
Use the "Apple LLVM" version number since the banner has changed in
newer versions of Mac OS X, resulting in the obsolete assembler path
being used to compile OpenSSL.

PR-URL: #5553
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Mar 21, 2016
Use the "Apple LLVM" version number since the banner has changed in
newer versions of Mac OS X, resulting in the obsolete assembler path
being used to compile OpenSSL.

PR-URL: #5553
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Mar 21, 2016
Use the "Apple LLVM" version number since the banner has changed in
newer versions of Mac OS X, resulting in the obsolete assembler path
being used to compile OpenSSL.

PR-URL: #5553
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
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.

6 participants