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 bsd build with gcc #16737

Closed
wants to merge 1 commit into from
Closed

build: fix bsd build with gcc #16737

wants to merge 1 commit into from

Conversation

mmarchini
Copy link
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 nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Nov 4, 2017
@gibfahn
Copy link
Member

gibfahn commented Nov 4, 2017

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
Copy link
Contributor Author

Changed it to ''

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

@mmarchini
Copy link
Contributor Author

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

maclover7
maclover7 approved these changes Nov 24, 2017
View reviewed changes
Copy link
Contributor

@maclover7 maclover7 left a comment

Choose a reason for hiding this comment

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

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

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

refack commented Nov 25, 2017

@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
Copy link
Contributor Author

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

refack
refack approved these changes Nov 27, 2017
View reviewed changes
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

💯

@refack
Copy link
Contributor

refack commented Nov 27, 2017

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

@refack refack added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 27, 2017
@refack
Copy link
Contributor

refack commented Nov 27, 2017

Landed in 4a7487b.

@refack refack closed this Nov 27, 2017
refack pushed a commit that referenced this pull request Nov 27, 2017
@refack
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 refack removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 28, 2017
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
@MylesBorins
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
@MylesBorins
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
@MylesBorins
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 MylesBorins mentioned this pull request Dec 12, 2017
Merged
gibfahn pushed a commit that referenced this pull request Dec 19, 2017
@gibfahn
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
Copy link
Member

gibfahn commented Dec 19, 2017

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
@gibfahn
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 gibfahn mentioned this pull request Dec 20, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@refack refack refack approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@jasnell jasnell jasnell approved these changes

@maclover7 maclover7 maclover7 approved these changes

@gibfahn gibfahn gibfahn 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.

node 8.6 assumes llvm on FreeBSD
7 participants
@mmarchini @gibfahn @refack @bnoordhuis @jasnell @maclover7 @nodejs-github-bot