account for CLANG_VENDOR when checking for llvm version #33860
Conversation
nodejs-github-bot
added
the
build
| @@ -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]+)") | |||
There was a problem hiding this comment.
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]+)")There was a problem hiding this comment.
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. 😄
There was a problem hiding this comment.
Sorry for late response.
The problem I pointed out does not occur now, because Apple LLVM >= 10 is the supported version.
addaleax
added
the
author ready
Fixes: #29536 PR-URL: #33860 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
|
Landed in 409fdba |
Fixes: #29536 PR-URL: #33860 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Fixes: #29536 PR-URL: #33860 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Fixes: #29536 PR-URL: #33860 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Fixes: #29536 PR-URL: #33860 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Fixes: #29536 PR-URL: #33860 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Checklist
Apologies if I'm forgetting something - this is my first commit to node. I didn't think it necessary to include testing results as no src code is actually modified; only the configure script.
Please see issue #29536 for more details. I have not verified if this breaks configurations on systems like
void-linuxwhere a CLANG_VENDOR` is not specified, but it did fix the issue on alpine linux. Before this PR is merged, I'd like to investigate that it can be built in such cases.In the meantime, it would help to possibly get other members to verify the change allows clang/llvm to be recognized by their various systems.