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.

Sign up for GitHub

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

account for CLANG_VENDOR when checking for llvm version #33860

Closed
wants to merge 1 commit into from
Closed

account for CLANG_VENDOR when checking for llvm version #33860

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion configure.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -805,7 +805,7 @@ def get_nasm_version(asm):

def get_llvm_version(cc):
return get_version_helper(
cc, r"(^(?:FreeBSD )?clang version|based on LLVM) ([0-9]+\.[0-9]+)")
cc, r"(^(?:.+ )?clang version|based on LLVM) ([0-9]+\.[0-9]+)")
Copy link
Contributor

Choose a reason for hiding this comment

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

For detecting the support of OpenSSL's functionality, configure.py checks xcode_version>=5.0 or llvm_version>=3.3 in https://github.com/nodejs/node/blob/master/configure.py#L1330
Thus, it changes the detection result for ex. Apple clang version 4.0.

If you need to check for Alpine, below one would be better.

- cc, r"(^(?:FreeBSD )?clang version|based on LLVM) ([0-9]+\.[0-9]+)")
+ cc, r"(^(?:(?:FreeBSD|Alpine) )?clang version|based on LLVM) ([0-9]+\.[0-9]+)")

Copy link
Contributor Author

@nathanblair nathanblair Jun 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

That's what this PR is intended to fix. It won't be feasible to hard code the regex string for specific CLANG_VENDORs. Because what if I want to compile clang myself and I set my CLANG_VENDOR to "MyClang". Now I have to recompile Nodejs for whatever distribution I'm using - and I haven't even changed anything specifically related to Nodejs's functionality or behavior. The check here only cares about returning a version string - it doesn't care about the vendor; so there should be no need to do anything other than match against a Word.

I also compiled nodejs using this branch on my Macbook Pro and it worked just fine. 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for late response.
The problem I pointed out does not occur now, because Apple LLVM >= 10 is the supported version.


def get_xcode_version(cc):
return get_version_helper(
Expand Down