Tune CC and CXX assignment #1336
Conversation
| make='gmake' | ||
| MAKE_CXX='CXX=c++' | ||
| elif [ "${NVM_OS}" = 'aix' ]; then | ||
| if [ "${NVM_OS}" = 'freebsd' ] || [ "${NVM_OS}" = 'aix' ]; then |
There was a problem hiding this comment.
I prefer to keep all the conditions for each OS separate - ie, i think it's more maintainable to repeat the make assignment rather than have multiple confusing code paths for the same OS.
| make='gmake' | ||
| fi | ||
| if nvm_has "clang++" && nvm_has "clang" && nvm_version_greater_than_or_equal_to nvm_clang_version 3.5 ; then | ||
| if [ "${NVM_OS}" = 'freebsd' ] || [ "${NVM_OS}" = 'darwin' ]; then | ||
| [ -z "${CXX-}" ] && MAKE_CXX='CC=cc CXX=c++' |
There was a problem hiding this comment.
this should use an if/else, or else it will fail on set -e.
Why is this being conditionally set?
There was a problem hiding this comment.
hmmm ... I didn't understand the being conditionally set means, would you mind to explain about this? Thanks.
There was a problem hiding this comment.
I'm asking why you're not always setting MAKE_CXX
There was a problem hiding this comment.
If the user set CXX and CC, we'll overwrite it and make it not working, so only when CXX and CC not set, we set it.
There was a problem hiding this comment.
right - we want to overwrite it, because it's often set to the wrong values.
There was a problem hiding this comment.
you may need to specify CXX=clang++-3.8 CC=clang-3.8
There was a problem hiding this comment.
nvm has always blindly overridden CC and CXX before, and i've never once seen a request to override those things. Why is now different?
There was a problem hiding this comment.
We ONLY set it for FreeBSD before, I don't know how many FreeBSD users are using nvm, but I guess most of the users is on Linux distros, and then MacOS, no CXX, CC were set in this two situations, so it makes sense there is no issue about it.
If we'll always overwrite CC and CXX, then export CC and CXX in .travis.yml makes no sense, why do we export that two variables there?
For distros like Ubuntu 12.04/14.04 doesn't have clang new enough, install clang from ppa or llvm official source will make the newer versions called clang-3.x, then nvm can't not detect it.
After all, what's the reason why we should not respect the environment variable? Do we really got a problem that the incorrect CXX and CC environment variable set made the build failed?
There was a problem hiding this comment.
So are we good to go now? Or any part need me to update? Thanks.
PeterDaveHello
force-pushed
the
fix-CC-CXX
branch
from
fc69b18 to
ae937ee
Compare
- Also set CC for Freebsd, not only CXX - Also set CC=cc, CXX=c++ on Mac OS by default as upstream Reference: https://github.com/nodejs/node/blob/1bd871655a8b76fa3be1e3c6c325efa74c86fcd9/configure#L16-L17
PeterDaveHello
force-pushed
the
fix-CC-CXX
branch
from
ae937ee to
5202645
Compare
| 'freebsd') | ||
| make='gmake' | ||
| if [ -z "${CXX-}" ]; then | ||
| MAKE_CXX='CC=cc CXX=c++' |
There was a problem hiding this comment.
do we instead want MAKE_CXX = "CC=${CC:-cc} CXX=${CXX:-c++}", without the condition?
There was a problem hiding this comment.
Will be the same result, right?
There was a problem hiding this comment.
I'm fine with MAKE_CXX = "CC=${CC:-cc} CXX=${CXX:-c++}"
There was a problem hiding this comment.
Slightly different - as-is, if "CC" is set but "CXX" is unset, then "CC" will be overridden - but if the reverse is true, "CXX" won't get set.
| ;; | ||
| 'darwin') | ||
| if [ -z "${CXX-}" ]; then | ||
| MAKE_CXX='CC=cc CXX=c++' |
There was a problem hiding this comment.
also here, do we instead want MAKE_CXX = "CC=${CC:-cc} CXX=${CXX:-c++}", without the condition?
| ;; | ||
| esac | ||
| if nvm_has "clang++" && nvm_has "clang" && nvm_version_greater_than_or_equal_to nvm_clang_version 3.5 \ | ||
| && [ -z "${CC-}" ] && [ -z "${CXX-}" ] ; then |
There was a problem hiding this comment.
what if one is specified but the other is not?
There was a problem hiding this comment.
hmmm... will be a little bit tricky?
There was a problem hiding this comment.
maybe also MAKE_CXX = "CC=${CC:-cc} CXX=${CXX:-c++}"?
PeterDaveHello
force-pushed
the
fix-CC-CXX
branch
from
5202645 to
01ff2b2
Compare
|
All updated! |
| case "${NVM_OS}" in | ||
| 'freebsd') | ||
| make='gmake' | ||
| MAKE_CXX="CC=${CC:-cc} CXX=${CXX:-c++}" |
There was a problem hiding this comment.
hm, this seems like an oversight - there's no local MAKE_CXX declaration in this function. let's add one above the case.
ljharb
force-pushed
the
fix-CC-CXX
branch
2 times, most recently
from
61123c2 to
4577149
Compare
No description provided.