tools: skip workaround for newer llvm by nanaya · Pull Request #14077 · nodejs/node · GitHub

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

I'm going to sleep but this seems to kind of work-ish maybe (barely compiles without this, and starts to compile with this - still running when creating this PR). Will update later if needed.

Fixes #14076.

LGTM if CI is green

Nit: put spaces around the < (if it works, and AFAIK it should)

nits:

put a . at the end of the two sentences
put them one after the other
put the URL last and start the line with Ref: https://...

Is there a chance that cc will be gcc? see my other comment

@nanaya thank you very much for your contribution 🥇

ignores clang on other platforms
assumes compiler is cc
assumes cc is clang
shell script

You could try and deduce if clang is used and which version it is in ./configure which is the script that creates config.gypi. Then you'll just need the 'conditions': [

Need to improve clang version detection

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

I noticed some weird thing here. The original goal was to allow building node-sass. @bnoordhuis suggested using llvm_version variable which is in generated config.gypi. It is indeed usable when building node.

But it doesn't seem to be used when building node-sass. The variable is in bundled config.gypi from here which is unpacked to ~/.node-gyp. I also tried modifying FreeBSD package's config.gypi (which somehow doesn't have llvm_version declared even though I'm pretty sure it's built with clang/llvm) but it's not read either.

(the above is using node from package)

Using llvm_version from config.gypi when building node does indeed work but it doesn't help much if it breaks building modules.

I'll report back later.

Using llvm_version seems fine so I updated it accordingly. I have no idea why it's not there when trying it before.

Fixes: nodejs#14076 Refs: https://svnweb.freebsd.org/ports/head/www/node/Makefile?revision=444555&view=markup

Looks like FreeBSD's packaged node doesn't have llvm_version defined in process.config for some reason.

LGTM, thanks. CI: https://ci.nodejs.org/job/node-test-pull-request/8986/

If using llvm_version is an issue for freebsd-ports, I'm okay with using your previous approach of shelling out but can you add an explaining comment in common.gypi in that case?

Also used in common.gypi to check whether a flag is needed or not based on llvm version.

FreeBSD ports disables bundled openssl by default and thus llvm_version is not assigned. I moved the assignment outside so it should be fine.

💯

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

Is this check future proof? Not sure how the < operator operates, but what happens if llvm_version == "10.0"? If it is a string comparison, '10.0' < '4.0'.

Good quastion, but in that case get_llvm_version would fail since it uses this RegEx r"(^(?:FreeBSD )?clang version|based on LLVM) ([3-9]\.[0-9]+)")
https://github.com/nodejs/node/blob/master/configure#L585

Ah, in that case don't worry about it. We'll have to update everything anyway when 10.0 comes.

I think this should be fine. Just tell me if there's anything else I need to do (rebase, commit message, code style, etc).

Updating llvm_version check to be more accurate would need a rework on its structure. Perhaps splitting it into llvm_version_major and llvm_version_minor or having some kind of version comparison function.

I also see deps/openssl/openssl.gypi is currently comparing llvm_version in similar way I did here.

I think this should be fine. Just tell me if there's anything else I need to do (rebase, commit message, code style, etc).

@nanaya thanks, this looks good as is.
CI: https://ci.nodejs.org/job/node-test-pull-request/9290/

Also used in common.gypi to check whether a flag is needed or not based on llvm version. PR-URL: nodejs#14077 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Refael Ackermann <refack@gmail.com>

PR-URL: nodejs#14077 Fixes: nodejs#14076 Refs: https://svnweb.freebsd.org/ports/head/www/node/Makefile?revision=444555&view=markup Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Refael Ackermann <refack@gmail.com>

Landed in fc544d4, 8979b4f

@nanaya thank you for your contribution 🥇 and congrats on being promoted by GitHub from:

to:

P.S. @nanaya for your next PR, if you want your full name to appear in the commit logs you could follow https://help.github.com/articles/setting-your-username-in-git/

Also used in common.gypi to check whether a flag is needed or not based on llvm version. PR-URL: #14077 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Refael Ackermann <refack@gmail.com>

PR-URL: #14077 Fixes: #14076 Refs: https://svnweb.freebsd.org/ports/head/www/node/Makefile?revision=444555&view=markup Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Refael Ackermann <refack@gmail.com>

Also used in common.gypi to check whether a flag is needed or not based on llvm version. PR-URL: #14077 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Refael Ackermann <refack@gmail.com>

PR-URL: #14077 Fixes: #14076 Refs: https://svnweb.freebsd.org/ports/head/www/node/Makefile?revision=444555&view=markup Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Refael Ackermann <refack@gmail.com>

Should this land on v6.x?
8979b4f would need to be manually backported fwiw

@refack do you think we should backport?

Should come with #16737 if it lands on 6.x

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

refack previously approved these changes Jul 5, 2017

View reviewed changes

refack reviewed Jul 5, 2017

View reviewed changes

nanaya force-pushed the freebsd-clang-4-compile branch from 34bb4a2 to fb13e57 Compare July 5, 2017 06:50

tools: skip workaround for newer llvm

71dcd6e

Fixes: nodejs#14076 Refs: https://svnweb.freebsd.org/ports/head/www/node/Makefile?revision=444555&view=markup

nanaya force-pushed the freebsd-clang-4-compile branch from fb13e57 to 71dcd6e Compare July 5, 2017 06:55

nanaya changed the title Don't add removed flag for freebsd 11.1+/clang 4+ tools: skip workaround for newer llvm Jul 5, 2017

bnoordhuis approved these changes Jul 5, 2017

View reviewed changes

tools: always include llvm_version in config

b2407bc

Also used in common.gypi to check whether a flag is needed or not based on llvm version.

refack approved these changes Jul 5, 2017

View reviewed changes

TimothyGu reviewed Jul 6, 2017

View reviewed changes

refack self-assigned this Jul 7, 2017

refack closed this Jul 22, 2017

addaleax mentioned this pull request Jul 24, 2017

v8.3.0 proposal #14452

Closed

addaleax mentioned this pull request Aug 2, 2017

v8.3.0 proposal #14594

Merged

MylesBorins added the lts-watch-v6.x label Aug 16, 2017

chmeeedalf mentioned this pull request Oct 17, 2017

node 8.6 assumes llvm on FreeBSD #16257

Closed

bnoordhuis mentioned this pull request Nov 30, 2017

can't build node-sass under FreeBSD 11.1 #17404

Closed

gibfahn mentioned this pull request Dec 19, 2017

build: fix bsd build with gcc #16737

Closed

2 tasks

refack removed their assignment Oct 12, 2018

-                      # https://lists.freebsd.org/pipermail/freebsd-toolchain/2016-March/002094.html
-                      'cflags': [ '-D_LIBCPP_TRIVIAL_PAIR_COPY_CTOR=1' ],
+                      'conditions': [
+                        ['llvm_version < "4.0"', {

tools: skip workaround for newer llvm by nanaya · Pull Request #14077 · nodejs/node · GitHub

Uh oh!

Uh oh!

tools: skip workaround for newer llvm - #14077

tools: skip workaround for newer llvm#14077nanaya wants to merge 2 commits intonodejs:masternodejs/node:masterfrom nanaya:freebsd-clang-4-compilenanaya/node:freebsd-clang-4-compileCopy head branch name to clipboard

Conversation

nanaya commented Jul 4, 2017 • edited by refack Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

refack Jul 5, 2017

Choose a reason for hiding this comment

Uh oh!

refack Jul 5, 2017

Choose a reason for hiding this comment

Uh oh!

refack Jul 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack commented Jul 5, 2017

Uh oh!

refack commented Jul 5, 2017

Uh oh!

refack commented Jul 5, 2017

Uh oh!

nanaya commented Jul 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nanaya commented Jul 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nanaya commented Jul 5, 2017

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

nanaya commented Jul 5, 2017

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

refack commented Jul 6, 2017

Uh oh!

TimothyGu Jul 6, 2017

Choose a reason for hiding this comment

Uh oh!

refack Jul 6, 2017

Choose a reason for hiding this comment

Uh oh!

TimothyGu Jul 6, 2017

Choose a reason for hiding this comment

Uh oh!

nanaya commented Jul 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

refack commented Jul 21, 2017

Uh oh!

refack commented Jul 22, 2017

Uh oh!

refack commented Jul 22, 2017

Uh oh!

refack commented Jul 22, 2017

Uh oh!

MylesBorins commented Aug 16, 2017

Uh oh!

MylesBorins commented Dec 19, 2017

Uh oh!

gibfahn commented Dec 19, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

tools: skip workaround for newer llvm#14077
nanaya wants to merge 2 commits into
nodejs:masterfrom
nanaya:freebsd-clang-4-compile

nanaya commented Jul 4, 2017 •
edited by refack
Loading

refack Jul 5, 2017 •
edited
Loading

nanaya commented Jul 5, 2017 •
edited
Loading

nanaya commented Jul 5, 2017 •
edited
Loading

nanaya commented Jul 15, 2017 •
edited
Loading