Skip to content

Change travis clone depth to 3 - #1270

Merged
joshbruce merged 2 commits into
markedjs:masterfrom
styfle:travis-depth
Jul 12, 2018
Merged

Change travis clone depth to 3#1270
joshbruce merged 2 commits into
markedjs:masterfrom
styfle:travis-depth

Conversation

@styfle

@styflestyfle commented May 13, 2018

Copy link
Copy Markdown
Member

Description

By default Travis CI clones the last 50 commits. Cloning less means faster checkout and clone times which results in slightly faster builds.

See the docs for more information.

Contributor

  • no tests required for this PR.

Committer

In most cases, this should be a different person than the contributor.

  • Draft GitHub release notes have been updated.
  • CI is green (no forced merge required).
  • Merge PR

@UziTech

Copy link
Copy Markdown
Member

Does the git depth need to be more than 1?

@styfle

Copy link
Copy Markdown
MemberAuthor

@UziTech Yes, according to the docs:

if you use a depth of 1 and have a queue of jobs, Travis CI won’t build commits that are in the queue when you push a new commit.

@styfle

Copy link
Copy Markdown
MemberAuthor

I could change to 3 if you are thinking it should be lower?

@UziTech

Copy link
Copy Markdown
Member

I was just thinking it should be as low as it can be. I don't know if there will be much of a difference in speed from 5 to 3

@styfle

Copy link
Copy Markdown
MemberAuthor

I have tried in a couple other repos and the speed difference is really small. It saves about 0.5 sec for each test from changing from depth 50 to 5.

@styfle

Copy link
Copy Markdown
MemberAuthor

I just changed from 5 to 3 so let's see if there is any measurable change.

@styflestyfle changed the title Change travis clone depth to 5Change travis clone depth to 3May 13, 2018
@styfle

styfle commented May 13, 2018

Copy link
Copy Markdown
MemberAuthor

Looks like the clone/checkout time went up for some reason.

It's hard to get good numbers because travis build times are widely different, even with zero changes to the config.

@styfle

Copy link
Copy Markdown
MemberAuthor

@UziTech Any other thoughts on this?

@UziTech

Copy link
Copy Markdown
Member

So, if I'm understanding this right, travis clones multiple commits so it can checkout the commit it needs to run the tests on in case there was another commit after the one travis is testing?

I don't think we need travis to pass on anything other than the last few commits. Then again .5 seconds faster on a 2 minute task doesn't save much.

@styfle

Copy link
Copy Markdown
MemberAuthor

travis clones multiple commits so it can checkout the commit it needs to run the tests on in case there was another commit after the one travis is testing

I'm not entirely sure about that but that seems correct based on the warning that depth=1 will skip the queued builds and just build the latest commit.

@DanielRuf Can you answer this question?

@DanielRuf

Copy link
Copy Markdown

I have tried in a couple other repos and the speed difference is really small. It saves about 0.5 sec for each test from changing from depth 50 to 5.

It highly depends on the sizes of the diffs between those commits and how much data they load. In big projects like Magento 2 you often see better results than in smaller projects. Generally: YMMV

@DanielRuf

Copy link
Copy Markdown

So, if I'm understanding this right, travis clones multiple commits so it can checkout the commit it needs to run the tests on in case there was another commit after the one travis is testing?

No, in general these shallow clones are meant to do some testing with detached heads and testing between a range of commits (if needed). Keeping the value low ensures better clone / checkout times in many projects.

@DanielRuf

Copy link
Copy Markdown

All commits except the first (root) only contain diffs when you work with Git.

@DanielRuf

Copy link
Copy Markdown

I'm not entirely sure about that but that seems correct based on the warning that depth=1 will skip the queued builds and just build the latest commit.

If you set it to 1 it waits until the build on this commit is done to trigger a new build for this specific commit. This can introduce issues in some cases. Anyway, 5 should be sufficient if it brings any visible improvements.

This + cache / node_modules / composer cache can dramatically boost the performance of builds.

@DanielRuf

DanielRuf commented May 15, 2018

Copy link
Copy Markdown

Right, not much room for improvement here as it is a relatively small project (except you have big commits, then you do not want to clone the big one always for the next 50 commits).

@UziTech

Copy link
Copy Markdown
Member

Travis CI clones repositories to a depth of 50 commits, which is only really useful if you are performing git operations.

Please note that if you use a depth of 1 and have a queue of jobs, Travis CI won’t build commits that are in the queue when you push a new commit.

@DanielRuf So travis uses the depth as how many jobs can be in the queue? I'm not understanding why we need more than 1. We aren't doing any git operations.

@UziTech

Copy link
Copy Markdown
Member

according to travis-ci/travis-ci#8321 (comment) :

git.depth is used when we clone the repo to run the build. If the commit you are trying to build is not reachable within the given depth at the time the repository is cloned during the build, it will fail.

We could really use Auto Cancellation and set the depth to 1 since we should never need to build anything other than the latest commit.

@UziTech

Copy link
Copy Markdown
Member

we may need to test out our minify step to make sure it still works

@DanielRuf

Copy link
Copy Markdown

@DanielRuf So travis uses the depth as how many jobs can be in the queue? I'm not understanding why we need more than 1. We aren't doing any git operations.

Not exactly. 1 would prevent multiple jobs with the same commit in them and would introduce some waiting time.5 is (still) enough. 1 can but does not have to produce any issues. Best practice is: do not use 1 but a greater value to allow builds for reused commits.

@DanielRuf

Copy link
Copy Markdown

we may need to test out our minify step to make sure it still works

Definitely.

@styfle

Copy link
Copy Markdown
MemberAuthor

@UziTech How can we test the minify step if it only runs on master? Merge this and see what happens?

@UziTech

Copy link
Copy Markdown
Member

Ya pretty much. I think it should be fine.

@styfle

Copy link
Copy Markdown
MemberAuthor

@joshbruce@davisjam Any objections?

zhenalexfan pushed a commit to zhenalexfan/MarkdownHan that referenced this pull request Nov 8, 2021
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@styfle@UziTech@DanielRuf@joshbruce