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

🏗 Clean up build cache on Travis #22926

Merged
merged 4 commits into from Jun 20, 2019
Merged

Conversation

estherkim
Copy link
Collaborator

Removes $HOME/.cache/pip from the cache because it changes every build, which means uploading new files for every Travis job. pip should already be supported on Xenial VMs but if not, we should install it in a before_install script instead of keeping in the cache.

Removes stop_sauce_connect.sh because it's a duplicate.

Also removes sauce_connect from the cache - unclear what that is?

Adds ps -ef as after_scripts on the remote test and e2e jobs, because they have a tendency to hang and I'd like to see what processes are still running after the tests are complete.

Copy link
Member

@danielrozenberg danielrozenberg left a comment

Choose a reason for hiding this comment

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

I believe sauce_connect is/used to be where we stored the sauce proxy binary. @rsimha for more context

.travis.yml Outdated
@@ -77,19 +77,19 @@ jobs:
script:
- unbuffer node build-system/pr-check/remote-tests.js
after_script:
- build-system/sauce_connect/stop_sauce_connect.sh
Copy link
Contributor

@rsimha rsimha Jun 19, 2019

Choose a reason for hiding this comment

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

This was intentionally duplicated because when a test task fails, the sauce connect proxy isn't stopped by the Travis job due to execOrDie. When tests pass, the second after_script call is a no-op.

.travis.yml Outdated
cache:
directories:
- node_modules
- build-system/tasks/e2e/node_modules
- build-system/tasks/visual-diff/node_modules
- sauce_connect
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the directory to which build-system/sauce_connect/start_sauce_connect.sh downloads the proxy. Caching it avoids a fresh download for each build.

- validator/node_modules
- validator/nodejs/node_modules
- validator/webui/node_modules
- $HOME/.cache/pip
Copy link
Contributor

Choose a reason for hiding this comment

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

Travis' native pip caching only works for python projects. $HOME/.cache/pip was cached based on these results.

From the logs, this line suggests that a cached install location is being used (even though, as you pointed out, the cache is updated at the end of the build). Curious: does the install time stay the same when caching is disabled?

Copy link
Collaborator Author

@estherkim estherkim Jun 19, 2019

Choose a reason for hiding this comment

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

Interesting repo! By removing it from the cache, pip install increases from 3s to 5s but store build cache decreases from 85s to 10s.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, good catch on your part in noticing the much slower store step!

Copy link
Contributor

@rsimha rsimha left a comment

Choose a reason for hiding this comment

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

LGTM after reverting the two sauce_connect changes.

Fixing pip caching, or alternatively, replacing pip with something else (context: #22528 (comment)) can be done separately.

@estherkim estherkim merged commit 92908d4 into ampproject:master Jun 20, 2019
@estherkim estherkim deleted the travis-cache branch June 20, 2019 14:55
thekorn pushed a commit to edelight/amphtml that referenced this pull request Sep 11, 2019
* Remove pip and sauce_connect from Travis cache, remove duplicate stop_sauce_connect script

* Add debugging for remote and e2e jobs

* readd sauce
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants