FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

build: fix llvm version detection in freebsd-10 by shigeki · Pull Request #11668 · nodejs/node · GitHub

/ node Public

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

Closed
shigeki wants to merge 3 commits into
nodejs:masterfrom
shigeki:clang_version_detect_freebsd10
ConversationCommits (3)ChecksFiles changed
Closed

build: fix llvm version detection in freebsd-10#11668
shigeki wants to merge 3 commits into
nodejs:masterfrom
shigeki:clang_version_detect_freebsd10

Conversation

shigeki commented Mar 3, 2017

Copy link
Copy Markdown
Contributor

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

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 added the build Issues and PRs related to build files or the CI. label Mar 3, 2017
shigeki added the freebsd Issues and PRs related to the FreeBSD platform. label Mar 3, 2017
bnoordhuis reviewed Mar 3, 2017
View reviewed changes
Comment thread configure Outdated
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]+)")

bnoordhuis Mar 3, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

You could simply remove the anchor, I think?

shigeki Mar 3, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

bnoordhuis Mar 3, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

shigeki Mar 3, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

bnoordhuis approved these changes Mar 3, 2017
View reviewed changes

bnoordhuis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM with a style nit.

Comment thread configure Outdated
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]+)")

bnoordhuis Mar 3, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

shigeki commented Mar 3, 2017

Copy link
Copy Markdown
Contributor Author

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

richardlau reviewed Mar 3, 2017
View reviewed changes
Comment thread configure Outdated
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) " +

richardlau Mar 3, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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

shigeki Mar 6, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

shigeki Mar 6, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Mar 6, 2017

Copy link
Copy Markdown
Member

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

shigeki commented Mar 7, 2017

Copy link
Copy Markdown
Contributor Author

@jasnell Thanks for running CI.

All is green. Landed in efaab8f. Thanks.

shigeki closed this Mar 7, 2017
shigeki added a commit that referenced this pull request Mar 7, 2017
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
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 mentioned this pull request Mar 8, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Apr 17, 2017
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
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 mentioned this pull request Apr 19, 2017
Merged
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

richardlau richardlau left review comments
bnoordhuis bnoordhuis approved these changes
jasnell jasnell approved these changes
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

Back | FazBrowse Home | New Git URL