-
-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[WIP][CI] Enable clang v3.8 on Travis CI #1310
base: master
Are you sure you want to change the base?
Conversation
PeterDaveHello
commented
Nov 17, 2016
•
edited
Loading
edited
- Clean up clang related codes and config
- Remove lldb-3.8 package since we don't need, though it's on llvm install instructions.
- The origin codes have a bug about the PATH which makes clang would not be detected/used correctly.
- Remove clang version print.
- Remove useless gcc/g++ related packages and variables.
9a1a5f8
to
9c1c0ba
Compare
.travis.yml
Outdated
- PATH=$(echo $PATH | sed 's/::/:/') | ||
- NVM_DIR="${TRAVIS_BUILD_DIR}" | ||
matrix: | ||
- SHELLCHECK=true | ||
- SHELL=bash TEST_SUITE=install_script | ||
- SHELL=sh TEST_SUITE=fast | ||
- SHELL=dash TEST_SUITE=fast |
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.
Um, what?
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.
You can enable your own repo on Travis and push branches to it without creating PRs, btw
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.
9c1c0ba
to
1c06619
Compare
1c06619
to
dbbf09e
Compare
- Remove lldb-3.8 package since we don't need, though it's on llvm instructions. - Fix clang PATH bug which makes clang would not be detected/used correctly. - Remove clang version print.
dbbf09e
to
4e1b8e8
Compare
@ljharb I fixed the conflicts 😄 |
.travis.yml
Outdated
- if [ -n "${SHELLCHECK-}" ]; then stack setup && stack install ShellCheck && shellcheck --version ; fi | ||
- if [ -z "${SHELLCHECK-}" ]; then 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 | ||
- mkdir -p $HOME/bin && ln -s /usr/bin/clang-3.8 $HOME/bin/clang && ln -s /usr/bin/clang++-3.8 $HOME/bin/clang++ |
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.
please surround all the paths in quotes, in case $HOME
has a space in it.
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.
Thanks!
4e1b8e8
to
69eda1c
Compare
Hmm - my main concern here is that now we won't be testing gcc-based installation. Test run speed is less important than testing the maximum amount of use cases. |
@ljharb to be honest, I don't think we need to test nodejs as its author will test it, on both gcc and clang, as they write the doc about compilation using gcc/clang, that's their job and we can do nothing, what we should do here is to make sure we pass the correct parameters to "make", am I right? |
The thing that needs testing is how nvm compiles it - not that node itself compiles, as it surely will. While it's a very fair point that simply asserting that we pass the correct parameters to |
So maybe just separate the "make" part to an individual test for both gcc and clang? Then We don't need to "make" nodejs so many times per CI round. |
Yes, I think that would be best - in other words:
That should cover all the testing, without adding any slow tests to the build. |
@ljharb what's the parameters should be tested in that? If we are not passing some special cases with potential problems, I really doubt if we need to build nodejs in the test. |
In numbers 3 and 4? We should test all the permutations we're likely to produce, but because |
I mean in number 1 and 2, as we won't bump the nodejs version to be tested automatically, a certain build with the same parameters has built successfully should always build successfully with the same parameters, can't we just mock this part by hard code? |
Sure, unless the tarballs change retroactively (as they have before). |
c6cfc3a
to
c20db2a
Compare