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

Improve npm in ci #1455

Merged
merged 3 commits into from
Mar 26, 2017
Merged

Improve npm in ci #1455

merged 3 commits into from
Mar 26, 2017

Conversation

PeterDaveHello
Copy link
Collaborator

This should make the CI faster.

@PeterDaveHello
Copy link
Collaborator Author

I'm also thinking about if we can just pin the nodejs version? Since I didn't see a requirement to install the latest version.

.travis.yml Outdated
@@ -18,6 +18,7 @@ cache:
- $HOME/.cabal
- $TRAVIS_BUILD_DIR/.cache
- $TRAVIS_BUILD_DIR/node_modules
- $TRAVIS_BUILD_DIR/versions
Copy link
Member

Choose a reason for hiding this comment

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

The benefit here would be "the amount of time it takes to untar the cache" - I intentionally did not cache this part - we might make all sorts of changes to an installed node version over the course of tests, and making tests deterministic is far more important than making them fast. Let's remove this commit.

@@ -17,6 +17,7 @@ cache:
- $HOME/.ghc
- $HOME/.cabal
- $TRAVIS_BUILD_DIR/.cache
- $TRAVIS_BUILD_DIR/node_modules
Copy link
Member

Choose a reason for hiding this comment

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

If we're going to do this, then after npm install, we need to always run npm prune.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure.

@ljharb ljharb added the testing Stuff related to testing nvm itself. label Mar 25, 2017
.travis.yml Outdated
before_install:
- $SHELL --version 2> /dev/null || dpkg -s $SHELL 2> /dev/null || which $SHELL
- curl --version
- wget --version
- if [ -n "${SHELLCHECK-}" ]; then cabal update && cabal install transformers-0.4.3.0 ShellCheck && shellcheck --version ; fi
- if [ -z "${SHELLCHECK-}" ]; then nvm install node && npm install && npm ls urchin doctoc; fi
- if [ -z "${SHELLCHECK-}" ]; then nvm install node && npm install && npm prune && npm ls urchin doctoc; fi
Copy link
Member

Choose a reason for hiding this comment

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

not a blocker, but it occurs to me that this step should probably go in install, not before_install?

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 agree, that's the place what it already been for a while, I can send another commit to move both npm and cabel into install section, what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@ljharb ljharb merged commit f344d06 into nvm-sh:master Mar 26, 2017
@PeterDaveHello PeterDaveHello deleted the improve-npm-in-ci branch March 26, 2017 07:20
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