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

build: fix bsd build with gcc by mmarchini · Pull Request #16737 · nodejs/node · GitHub

/ node Public

build: fix bsd build with gcc - #16737

Closed
mmarchini wants to merge 1 commit into
nodejs:masterfrom
mmarchini:fix-bsd-build-8.6.x
ConversationCommits (1)ChecksFiles changed
Closed

build: fix bsd build with gcc#16737
mmarchini wants to merge 1 commit into
nodejs:masterfrom
mmarchini:fix-bsd-build-8.6.x

Conversation

mmarchini commented Nov 4, 2017

Copy link
Copy Markdown
Contributor

BSD build with GCC was breaking because it was checking for the llvm_version
variable on common.gypi, even though LLVM wasn't installed (or needed). When
LLVM wasn't installed, llvm_version wasn't being defined, causing the build to break.

Fixes: #16257

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

build

nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Nov 4, 2017

gibfahn commented Nov 4, 2017

Copy link
Copy Markdown
Member

Approach looks correct to me, but we don't use 'null' elsewhere, we seem to use '' for empty/blank values, so that might be a better idea.

cc/ @nodejs/gyp

mmarchini force-pushed the fix-bsd-build-8.6.x branch from e58aa6e to 803871c Compare November 4, 2017 13:07

mmarchini commented Nov 4, 2017

Copy link
Copy Markdown
Contributor Author

Changed it to ''

bnoordhuis approved these changes Nov 4, 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, thanks. CI: https://ci.nodejs.org/job/node-test-pull-request/11187/

mmarchini commented Nov 6, 2017

Copy link
Copy Markdown
Contributor Author

ARM build failed but it doesn't seem to be related to these changes (see nodejs/build#173)

maclover7 approved these changes Nov 24, 2017
View reviewed changes

maclover7 left a comment

Copy link
Copy Markdown
Contributor

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

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

refack added the freebsd Issues and PRs related to the FreeBSD platform. label Nov 25, 2017

refack commented Nov 25, 2017

Copy link
Copy Markdown
Contributor

@mmarchini could you remove this line as well.

node/deps/openssl/openssl.gyp

Line 10 in ad80c21

'llvm_version%': 0,

And maybe check if

o['variables']['llvm_version'] = get_llvm_version(CC) if is_clang else 0
['"0" < llvm_version < "4.0"]

works?

build: fix bsd build with gcc
62d7469 
BSD build with GCC was broken because it was checking for the llvm_version
variable on common.gypi, even though llvm wasn't installed (or needed). If
LLVM wasn't installed, llvm_version wasn't being defined, breaking the build.

Fixes: nodejs#16257

mmarchini commented Nov 27, 2017

Copy link
Copy Markdown
Contributor Author

Yes, it works. Thanks. I've also rebased the branch.

refack approved these changes Nov 27, 2017
View reviewed changes

refack left a comment

Copy link
Copy Markdown
Contributor

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

💯

refack commented Nov 27, 2017

Copy link
Copy Markdown
Contributor

CI: https://ci.nodejs.org/job/node-test-commit/14396/

refack added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Nov 27, 2017

refack commented Nov 27, 2017

Copy link
Copy Markdown
Contributor

Landed in 4a7487b.

refack closed this Nov 27, 2017
refack pushed a commit that referenced this pull request Nov 27, 2017
build: fix bsd build with gcc
4a7487b 
BSD build with GCC was broken because it was checking for the
llvm_version variable on common.gypi, even though llvm wasn't
installed (or needed).

PR-URL: #16737
Fixes: #16257
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Refael Ackermann <refack@gmail.com>
refack removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Nov 28, 2017
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
build: fix bsd build with gcc
304dfab 
BSD build with GCC was broken because it was checking for the
llvm_version variable on common.gypi, even though llvm wasn't
installed (or needed).

PR-URL: #16737
Fixes: #16257
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Refael Ackermann <refack@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
build: fix bsd build with gcc
61a6a25 
BSD build with GCC was broken because it was checking for the
llvm_version variable on common.gypi, even though llvm wasn't
installed (or needed).

PR-URL: #16737
Fixes: #16257
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Refael Ackermann <refack@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
build: fix bsd build with gcc
0a40a11 
BSD build with GCC was broken because it was checking for the
llvm_version variable on common.gypi, even though llvm wasn't
installed (or needed).

PR-URL: #16737
Fixes: #16257
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Refael Ackermann <refack@gmail.com>
MylesBorins mentioned this pull request Dec 12, 2017
Merged
gibfahn pushed a commit that referenced this pull request Dec 19, 2017
build: fix bsd build with gcc
986e439 
BSD build with GCC was broken because it was checking for the
llvm_version variable on common.gypi, even though llvm wasn't
installed (or needed).

PR-URL: #16737
Fixes: #16257
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Refael Ackermann <refack@gmail.com>

gibfahn commented Dec 19, 2017

Copy link
Copy Markdown
Member

Fix for #14077, should land on 6.x if that lands

This was referenced Dec 19, 2017
Closed
Closed
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
build: fix bsd build with gcc
134bbd8 
BSD build with GCC was broken because it was checking for the
llvm_version variable on common.gypi, even though llvm wasn't
installed (or needed).

PR-URL: #16737
Fixes: #16257
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Refael Ackermann <refack@gmail.com>
gibfahn mentioned this pull request Dec 20, 2017
Merged
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

bnoordhuis bnoordhuis approved these changes
jasnell jasnell approved these changes
+3 more reviewers
refack refack approved these changes
maclover7 maclover7 approved these changes
gibfahn gibfahn approved these changes
Reviewers whose approvals may not affect merge requirements

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.

node 8.6 assumes llvm on FreeBSD

7 participants

Back | FazBrowse Home | New Git URL