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

Jump to bottom

build: fix llvm version detection in freebsd-10 #11668

Closed
wants to merge 3 commits into from
Closed

build: fix llvm version detection in freebsd-10 #11668

wants to merge 3 commits into from

Conversation

shigeki
Copy link
Contributor

@shigeki shigeki commented Mar 3, 2017

In FreeBSD-10, the banner of clang version start with "FreeBSD clang version" but the current configure checks /^clang version/ so that llvm_version is 0 and openssl is built with asm_obsolete.

This can be seen in the CI output of https://ci.nodejs.org/job/node-test-commit-freebsd/7417/nodes=freebsd10-64/consoleFull that 'llvm_version': 0, in the current configure output.

With this fix, the clang version banner can be checked properly in FreeBSD-10. The result of https://ci.nodejs.org/job/node-test-commit-freebsd/7419/nodes=freebsd10-64/consoleFull shows that 'llvm_version': '3.4',.

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

build

CC: @nodejs/platform-freebsd or @nodejs/build

@shigeki
build: fix llvm version detection in freebsd-10
b01b834 
In FreeBSD-10, the banner of clang version is "FreeBSD clang version".
Fix regex to detect it.
@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Mar 3, 2017
@shigeki shigeki added the freebsd Issues and PRs related to the FreeBSD platform. label Mar 3, 2017
bnoordhuis
bnoordhuis reviewed Mar 3, 2017
View reviewed changes
configure Outdated
@@ -576,7 +576,8 @@ def get_version_helper(cc, regexp):

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

Choose a reason for hiding this comment

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

You could simply remove the anchor, I think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought it could be fixed but have no full confirmation if it has no side effects. This is very conservative fix not to break existing checks.

Copy link
Member

@bnoordhuis bnoordhuis Mar 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I suppose a little conservatism won't hurt. Can you line up the strings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm a little coward ;-).
get_xcode_version in 4 lines below had also 4 space indents. I fixed both. Thanks.

bnoordhuis
bnoordhuis approved these changes Mar 3, 2017
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM with a style nit.

configure Outdated
@@ -576,7 +576,8 @@ def get_version_helper(cc, regexp):

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

@bnoordhuis bnoordhuis Mar 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I suppose a little conservatism won't hurt. Can you line up the strings?

@shigeki
Copy link
Contributor Author

shigeki commented Mar 3, 2017

CI was done in https://ci.nodejs.org/job/node-test-pull-request/6682/ and all is green.

richardlau
richardlau reviewed Mar 3, 2017
View reviewed changes
configure Outdated
@@ -576,11 +576,12 @@ def get_version_helper(cc, regexp):

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

@richardlau richardlau Mar 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

An alternative would be a non-capturing group, e.g.

cc, r"(^(?:FreeBSD )?clang version|based on LLVM) " +

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not think of a non-capturing parentheses. It does not break existing check and looks better. Thanks.
@bnoordhuis I fixed in ed1d266. PTAL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Confirmed it works fine in both FreeBSD-10 of clang-3.4 with OS prefix in its banner, https://ci.nodejs.org/job/node-test-commit-freebsd/7474/nodes=freebsd10-64/consoleFull and FreeBSD-11 of clang-3.6 without no OS prefix ,https://ci.nodejs.org/job/node-test-commit-freebsd/7474/nodes=freebsd11-x64/consoleFull .

@jasnell
Copy link
Member

jasnell commented Mar 6, 2017

New CI: https://ci.nodejs.org/job/node-test-pull-request/6733/

@shigeki
Copy link
Contributor Author

shigeki commented Mar 7, 2017

@jasnell Thanks for running CI.

All is green. Landed in efaab8f. Thanks.

@shigeki shigeki closed this Mar 7, 2017
shigeki added a commit that referenced this pull request Mar 7, 2017
@shigeki
build: fix llvm version detection in freebsd-10
efaab8f 
In FreeBSD-10, the banner of clang version is "FreeBSD clang version".
Fix regex to detect it.

PR-URL: #11668
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
evanlucas pushed a commit that referenced this pull request Mar 8, 2017
@shigeki @evanlucas
build: fix llvm version detection in freebsd-10
e5b530c 
In FreeBSD-10, the banner of clang version is "FreeBSD clang version".
Fix regex to detect it.

PR-URL: #11668
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@evanlucas evanlucas mentioned this pull request Mar 8, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Apr 17, 2017
@shigeki @MylesBorins
build: fix llvm version detection in freebsd-10
a31d6e2 
In FreeBSD-10, the banner of clang version is "FreeBSD clang version".
Fix regex to detect it.

PR-URL: #11668
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
@shigeki @MylesBorins
build: fix llvm version detection in freebsd-10
a899b0b 
In FreeBSD-10, the banner of clang version is "FreeBSD clang version".
Fix regex to detect it.

PR-URL: #11668
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@MylesBorins MylesBorins mentioned this pull request Apr 19, 2017
Merged
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
@shigeki @andrew749
build: fix llvm version detection in freebsd-10
a94eb66 
In FreeBSD-10, the banner of clang version is "FreeBSD clang version".
Fix regex to detect it.

PR-URL: nodejs/node#11668
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
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
Reviewers

@richardlau richardlau richardlau left review comments

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@jasnell jasnell jasnell approved these changes

@cjihrig cjihrig cjihrig approved these changes

Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. freebsd Issues and PRs related to the FreeBSD platform.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@shigeki @jasnell @bnoordhuis @cjihrig @richardlau @MylesBorins @nodejs-github-bot