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.

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

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

PeterDaveHello
Copy link
Collaborator

@PeterDaveHello PeterDaveHello commented Nov 17, 2016

  • 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.

@PeterDaveHello PeterDaveHello force-pushed the clang-on-travis branch 5 times, most recently from 9a1a5f8 to 9c1c0ba Compare November 17, 2016 14:12
.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
Copy link
Member

Choose a reason for hiding this comment

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

Um, what?

Copy link
Member

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

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 yes I just did, that's what I did before that, and just found the conflicts with #1311, so just wait for that, sorry about this.

@PeterDaveHello PeterDaveHello changed the title [WIP] enable clang v3.5+ on travis [CI] Enable clang v3.8 on Travis CI Nov 17, 2016
- 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.
@PeterDaveHello
Copy link
Collaborator Author

@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++
Copy link
Member

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks!

@ljharb
Copy link
Member

ljharb commented Nov 17, 2016

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.

@PeterDaveHello
Copy link
Collaborator Author

@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?

@ljharb
Copy link
Member

ljharb commented Nov 18, 2016

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 make is a great unit test - and we definitely should add "fast" unit tests for nvm install -s that stubs out nvm_download_artifact and make; that's a wonderful idea - but what that fails to do is integration test nvm install -s on gcc, which is the purpose of the test.

@PeterDaveHello
Copy link
Collaborator Author

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.

@ljharb
Copy link
Member

ljharb commented Nov 18, 2016

Yes, I think that would be best - in other words:

  1. run at least one nvm install -s test with gcc
  2. run at least one nvm install -s test with clang
  3. make unit tests that stub things out and test calls to make for gcc
  4. make unit tests that stub things out and test calls to make for clang

That should cover all the testing, without adding any slow tests to the build.

@PeterDaveHello
Copy link
Collaborator Author

@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.

@ljharb
Copy link
Member

ljharb commented Nov 18, 2016

In numbers 3 and 4? We should test all the permutations we're likely to produce, but because make will be stubbed out, we won't need to build node at all for those (just mock out some of the filesystem stuff that the code expects)

@PeterDaveHello
Copy link
Collaborator Author

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?

@ljharb
Copy link
Member

ljharb commented Nov 18, 2016

Sure, unless the tarballs change retroactively (as they have before).

@ljharb ljharb added the testing Stuff related to testing nvm itself. label Sep 29, 2021
@ljharb ljharb marked this pull request as draft September 29, 2021 05:43
@ljharb ljharb force-pushed the master branch 2 times, most recently from c6cfc3a to c20db2a Compare June 10, 2024 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing Stuff related to testing nvm itself.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants