Do not remove src dir after compilation #1299

PeterDaveHello · 2016-11-13T19:48:43Z

This will help speed up the installation time for the install from source and non-first time
installation as make can use the "cache", this can speed up the build time and test time on
Travis-CI as we'll cache .cache dir.

Ref:

$ time nvm i -s 6.9.1
.
.
.
Now using node v6.9.1 (npm v3.10.8)
real    0m31.914s
user    0m28.116s
sys     0m4.344s

…eeds This will help speed up the installation time for the non-first time installation, especially can speed up the build time and test time on Travis-CI as we'll cache .cache dir.

PeterDaveHello · 2016-11-13T21:07:56Z

To prove that the change can speed up the build time / CI test time, just send another commit come after the main change, as you'll rebase then merge PRs, it won't hurt at all.

PeterDaveHello · 2016-11-13T21:10:35Z

As you can see, we usually take about more than 2.5 hours to pass per round of CI test:

We need only less than one hour now:

(and about 1/3 in real elapsed time)

ljharb · 2016-11-14T07:37:49Z

CONTRIBUTING.md

@@ -7,7 +7,6 @@ However, before submitting, please review the following:
 - Please include tests. Changes with tests will be merged very quickly.
 - Please manually confirm that your changes work in `bash`, `sh`/`dash`, `ksh`, and `zsh`. Fast tests do run in these shells, but it's nice to manually verify also.
 - Please maintain consistent whitespace - 2-space indentation, trailing newlines in all files, etc.
- - Any time you make a change to your PR, please rebase freshly on top of master. Nobody likes merge commits.


Please revert this. We absolutely will not use Github's rebase, as that destroys history. The rebase must be done on the command line.

@ljharb I reverted, but how did you merge the PRs on GitHub? Squash and merge?

I merge them on the command line, and with the "allow edits from maintainers" checkbox, I'm able to rebase PRs for people (and force push to their branch) and then fastforward-merge their PR - whereas Github's "rebase" or "squash" feature severs the connection to the actual git log that's on the PR.

ljharb · 2016-11-14T07:39:28Z

nvm.sh

@@ -1883,8 +1883,7 @@ nvm_install_source() {
    ./configure --prefix="${VERSION_PATH}" $ADDITIONAL_PARAMETERS && \
    $make -j "${NVM_MAKE_JOBS}" ${MAKE_CXX-} && \
    command rm -f "${VERSION_PATH}" 2>/dev/null && \
-    $make -j "${NVM_MAKE_JOBS}" ${MAKE_CXX-} install && \
-    command rm -rf "${TMPDIR}"


is this dir cleared by nvm cache clear?

@ljharb yes, it'll be cleared by nvm cache clear

I think I'd still like to remove the dir if the installation fails. Can we move this down to line 1906?

@ljharb for installation fails, would leave this dir for debug would be even better?

That's never been a problem before - usually the error message is sufficient, and since the majority of users won't have the skills to debug, the ones that will also have the skills to go comment out the rm line (as they do now)

I update it 👍

ljharb · 2016-11-14T21:37:36Z

nvm.sh

@@ -1904,6 +1904,8 @@ nvm_install_source() {
  fi

  nvm_err "nvm: install ${VERSION} failed!"
+  # shellcheck disable=SC2031


by my reading of https://github.com/koalaman/shellcheck/wiki/SC2031, this shellcheck error is actually very serious and should not be bypassed.

I think that in order for TMPDIR to be available outside of the if chain, we need to refactor that chain so that it's defined at the function level.

@ljharb I move that part out of the if, is that good for you? The function would be still the same.

PeterDaveHello · 2016-11-16T05:34:58Z

@ljharb anything I need to do here? Since this can totally decrease the CI build time, I hope this can be merged as soon as possible 😄

[caching] nvm install: Do not remove src dir after compilation succ…

0360829

…eeds This will help speed up the installation time for the non-first time installation, especially can speed up the build time and test time on Travis-CI as we'll cache .cache dir.

PeterDaveHello force-pushed the enhance-cache branch from d389b18 to 8dac2c3 Compare November 13, 2016 20:41

ljharb reviewed Nov 14, 2016

View reviewed changes

ljharb requested changes Nov 14, 2016

View reviewed changes

ljharb added the installing node Issues with installing node/io.js versions. label Nov 14, 2016

PeterDaveHello force-pushed the enhance-cache branch 2 times, most recently from 8dac2c3 to 0704ecb Compare November 14, 2016 08:50

ljharb approved these changes Nov 14, 2016

View reviewed changes

ljharb force-pushed the enhance-cache branch 2 times, most recently from 9a02dd0 to f045ddd Compare November 14, 2016 08:55

PeterDaveHello force-pushed the enhance-cache branch from f045ddd to 38ba71a Compare November 14, 2016 10:06

ljharb requested changes Nov 14, 2016

View reviewed changes

PeterDaveHello force-pushed the enhance-cache branch from 38ba71a to c600b8c Compare November 15, 2016 09:21

ljharb approved these changes Nov 16, 2016

View reviewed changes

ljharb force-pushed the enhance-cache branch from c600b8c to 0360829 Compare November 16, 2016 06:45

ljharb merged commit 0360829 into nvm-sh:master Nov 16, 2016

PeterDaveHello deleted the enhance-cache branch November 16, 2016 08:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not remove src dir after compilation #1299

Do not remove src dir after compilation #1299

PeterDaveHello commented Nov 13, 2016 •

edited

Loading

PeterDaveHello commented Nov 13, 2016

PeterDaveHello commented Nov 13, 2016 •

edited

Loading

ljharb Nov 14, 2016

PeterDaveHello Nov 14, 2016 •

edited

Loading

ljharb Nov 14, 2016

ljharb Nov 14, 2016

PeterDaveHello Nov 14, 2016

ljharb Nov 14, 2016

PeterDaveHello Nov 14, 2016

ljharb Nov 14, 2016

PeterDaveHello Nov 14, 2016

ljharb Nov 14, 2016

PeterDaveHello Nov 15, 2016

PeterDaveHello commented Nov 16, 2016

Do not remove src dir after compilation #1299

Do not remove src dir after compilation #1299

Conversation

PeterDaveHello commented Nov 13, 2016 • edited Loading

PeterDaveHello commented Nov 13, 2016

PeterDaveHello commented Nov 13, 2016 • edited Loading

Choose a reason for hiding this comment

PeterDaveHello Nov 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PeterDaveHello commented Nov 16, 2016

PeterDaveHello commented Nov 13, 2016 •

edited

Loading

PeterDaveHello commented Nov 13, 2016 •

edited

Loading

PeterDaveHello Nov 14, 2016 •

edited

Loading