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

[New] Use Clang as C/C++ compiler if we detected it, close #902 #1300

Merged
merged 3 commits into from
Nov 17, 2016
Merged

[New] Use Clang as C/C++ compiler if we detected it, close #902 #1300

merged 3 commits into from
Nov 17, 2016

Conversation

PeterDaveHello
Copy link
Collaborator

@PeterDaveHello PeterDaveHello commented Nov 13, 2016 •
edited by ljharb
Loading

Fixes #902.

@PeterDaveHello
Copy link
Collaborator Author

Looks like we need clang v3.5+

@PeterDaveHello
Copy link
Collaborator Author

PeterDaveHello commented Nov 14, 2016 •
edited
Loading

Looks like clang version output may have different format:

Ubuntu 14.04:

Ubuntu clang version 3.4-1ubuntu3 (tags/RELEASE_34/final) (based on LLVM 3.4)
Target: x86_64-pc-linux-gnu
Thread model: posix

Ubuntu 15.04:

Ubuntu clang version 3.6.0-2ubuntu1 (tags/RELEASE_360/final) (based on LLVM 3.6.0)
Target: x86_64-pc-linux-gnu
Thread model: posix

Ubuntu 16.04:

clang version 3.8.0-2ubuntu4 (tags/RELEASE_380/final)
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin

FreeBSD 10.3:

FreeBSD clang version 3.4.1 (tags/RELEASE_34/dot1-final 208032) 20140512
Target: x86_64-unknown-freebsd10.3
Thread model: posix

ArchLinux:

clang version 3.9.0 (tags/RELEASE_390/final)
Target: x86_64-unknown-linux-gnu
Thread model: posix
InstalledDir: /usr/sbin

Debian 8.6:

Debian clang version 3.5.0-10 (tags/RELEASE_350/final) (based on LLVM 3.5.0)
Target: x86_64-pc-linux-gnu
Thread model: posix

@PeterDaveHello PeterDaveHello force-pushed the clang-llvm branch 8 times, most recently from 05b0bc7 to 7e9e2ca Compare November 14, 2016 07:14
ljharb
ljharb reviewed Nov 14, 2016
View reviewed changes
.travis.yml
- if [ -n "${SHELLCHECK-}" ]; then sudo apt-key adv --keyserver hkp://keyserver.ubuntu.com:80 --recv-keys 575159689BEFB442 && echo 'deb http://download.fpcomplete.com/ubuntu precise main' | sudo tee /etc/apt/sources.list.d/fpco.list && sudo apt-get update && sudo apt-get install stack bc -y && stack setup && stack install ShellCheck && shellcheck --version ; fi
- if [ -z "${SHELLCHECK-}" ]; then wget -O - http://apt.llvm.org/llvm-snapshot.gpg.key | sudo apt-key add - && echo -e "deb http://apt.llvm.org/precise/ llvm-toolchain-precise main\ndeb http://apt.llvm.org/precise/ llvm-toolchain-precise-3.8 main" | sudo tee /etc/apt/sources.list.d/clang.list && sudo apt-get update && sudo apt-get install clang-3.8 lldb-3.8 -y --force-yes && sudo ln -sf /usr/bin/clang-3.8 /usr/bin/clang && sudo ln -sf /usr/bin/clang++-3.8 /usr/bin/clang++ && clang --version ; fi
Copy link
Member

Choose a reason for hiding this comment

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

are these not things that can be installed by travis in the "addons: apt" section above?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

clang in Ubuntu 12.04 is too old ( < v3.5)

Copy link
Member

Choose a reason for hiding this comment

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

Will this installation process be cached, like the shellcheck build is?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think so, do we have cache on the apt packages?

Copy link
Member

Choose a reason for hiding this comment

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

Yes - https://github.com/creationix/nvm/blob/master/.travis.yml#L12-L15

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this only cache the ~/.stack, not the package in system level, am I right?

Copy link
Member

Choose a reason for hiding this comment

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

The shellcheck build is very very slow, and once the first one was built, it didn't need to be rebuilt on successive builds.

I want to ensure that we don't need to build clang more than the once.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We install clang from binary, no build needed.

Copy link
Member

Choose a reason for hiding this comment

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

ah ok, that's good. is caching the download worth it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think so, as the time will be mainly used to solve the dependencies, not on the package downloading.

nvm.sh
@@ -192,6 +192,10 @@ nvm_rc_version() {
fi
}

nvm_clang_version() {
Copy link
Member

Choose a reason for hiding this comment

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

let's add some unit tests for this

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ljharb where should I put the test script? Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

test/fast/Unit Tests

nvm.sh
@@ -1854,6 +1858,10 @@ nvm_install_source() {
elif [ "${NVM_OS}" = 'aix' ]; then
make='gmake'
fi
if nvm_has "clang++" && nvm_has "clang" && nvm_version_greater_than_or_equal_to nvm_clang_version 3.5 ; then
Copy link
Member

Choose a reason for hiding this comment

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

are both clang and clang++ needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess so: https://github.com/nodejs/node/blob/master/BUILDING.md#unix--os-x

@ljharb ljharb added feature requests I want a new feature in nvm! installing node Issues with installing node/io.js versions. labels Nov 14, 2016
@PeterDaveHello PeterDaveHello force-pushed the clang-llvm branch 6 times, most recently from e96cd9b to f6d8f04 Compare November 14, 2016 17:41
@PeterDaveHello
Copy link
Collaborator Author

@ljharb I think we are good to go now 😄

@ljharb ljharb merged commit ab6be9c into nvm-sh:master Nov 17, 2016
@PeterDaveHello PeterDaveHello deleted the clang-llvm branch November 17, 2016 08:19
edwmurph pushed a commit to edwmurph/nvm that referenced this pull request Apr 9, 2018
@ljharb
Merge pull request nvm-sh#1300 from PeterDaveHello/clang-llvm
23cb938 
[New] `nvm install -s`: Use clang as C/C++ compiler if detected

Fixes nvm-sh#902
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljharb ljharb ljharb left review comments

Assignees
No one assigned
Labels
feature requests I want a new feature in nvm! installing node Issues with installing node/io.js versions.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@PeterDaveHello @ljharb