-
Notifications
You must be signed in to change notification settings - Fork 30k
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.
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
tools: skip workaround for newer llvm #14077
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM if CI is green
common.gypi
Outdated
'ldflags': [ | ||
'-Wl,--export-dynamic', | ||
], | ||
'conditions': [ | ||
['clang_major_version<4', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: put spaces around the <
(if it works, and AFAIK it should)
common.gypi
Outdated
'ldflags': [ | ||
'-Wl,--export-dynamic', | ||
], | ||
'conditions': [ | ||
['clang_major_version<4', { | ||
# Use this flag because on FreeBSD std::pairs copy constructor is non-trivial |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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://...
common.gypi
Outdated
@@ -65,6 +65,9 @@ | |||
}, { | |||
'clang%': 0, | |||
}], | |||
['OS=="freebsd"', { | |||
'clang_major_version': '<!(echo "__clang_major__" | cc -E -P -)', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a chance that see my other commentcc
will be gcc
?
@nanaya thank you very much for your contribution 🥇 |
You could try and deduce if |
I noticed some weird thing here. The original goal was to allow building node-sass. @bnoordhuis suggested using But it doesn't seem to be used when building node-sass. The variable is in bundled (the above is using node from package) Using I'll report back later. |
34bb4a2
to
fb13e57
Compare
Using |
fb13e57
to
71dcd6e
Compare
Looks like FreeBSD's packaged node doesn't have |
There was a problem hiding this 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/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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
# https://lists.freebsd.org/pipermail/freebsd-toolchain/2016-March/002094.html | ||
'cflags': [ '-D_LIBCPP_TRIVIAL_PAIR_COPY_CTOR=1' ], | ||
'conditions': [ | ||
['llvm_version < "4.0"', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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'
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 I also see |
@nanaya thanks, this looks good as is. |
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>
@nanaya thank you for your contribution 🥇 and congrats on being promoted by GitHub from: |
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? |
@refack do you think we should backport? |
Should come with #16737 if it lands on 6.x |
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected 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.