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.

Sign up for GitHub

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

Jump to bottom

[WIP] download archive instead of cloning repo in integration tests #4703

Closed
wants to merge 3 commits into from
Closed

[WIP] download archive instead of cloning repo in integration tests #4703

wants to merge 3 commits into from

Conversation

tesuji
Copy link
Contributor

@tesuji tesuji commented Oct 19, 2019

changelog: none

@tesuji
Copy link
Contributor Author

tesuji commented Oct 19, 2019

It works.

@phansch
Copy link
Member

phansch commented Oct 19, 2019

@bors try

bors added a commit that referenced this pull request Oct 19, 2019
@bors
Auto merge of #4703 - lzutao:donot-clone, r=<try>
db81538 
download archive instead of cloning repo in integration tests

changelog: none
@bors
Copy link
Collaborator

bors commented Oct 19, 2019

⌛ Trying commit 83c227e with merge db81538...

@tesuji
Copy link
Contributor Author

tesuji commented Oct 19, 2019 •
edited
Loading

Note that this PR will not lead to good win in build time. But it will not lead to
build regression neither.

@bors
Copy link
Collaborator

bors commented Oct 19, 2019

☀️ Try build successful - checks-travis, status-appveyor
Build commit: db81538 (db81538951184112f4eb242af50290cff24e2fc1)

@phansch
Copy link
Member

phansch commented Oct 20, 2019

The integration tests took ~20 minutes instead of the usual 10 minutes this time. Let's try again; maybe a caching issue?

@bors try

@bors
Copy link
Collaborator

bors commented Oct 20, 2019

⌛ Trying commit 83c227e with merge 8dcdeff...

bors added a commit that referenced this pull request Oct 20, 2019
@bors
Auto merge of #4703 - lzutao:donot-clone, r=<try>
8dcdeff 
download archive instead of cloning repo in integration tests

changelog: none
@tesuji
Copy link
Contributor Author

tesuji commented Oct 20, 2019 •
edited
Loading

maybe a caching issue?

Yes, it is. Because of #4692 , the cache in try branch was not updated.

@bors
Copy link
Collaborator

bors commented Oct 20, 2019

☀️ Try build successful - checks-travis, status-appveyor
Build commit: 8dcdeff (8dcdeff1b8f5996a4f35beb069bab06cad6729e5)

@tesuji tesuji changed the title download archive instead of cloning repo in integration tests [WIP] download archive instead of cloning repo in integration tests Oct 20, 2019
@tesuji
Copy link
Contributor Author

tesuji commented Oct 20, 2019

I will investigate the regression in build time later.

flip1995
flip1995 reviewed Oct 21, 2019
View reviewed changes
ci/integration-tests.sh
@@ -10,8 +10,9 @@ cargo install --force --debug --path .

echo "Running integration test for crate ${INTEGRATION}"

git clone --depth=1 "https://github.com/${INTEGRATION}.git" checkout
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't this also be done with --single-branch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. But downloading tar.gz archives is about 1 second faster than using git.

Copy link
Member

Choose a reason for hiding this comment

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

So 1 second in a ~8 minute build? I think just using git should be ok then?

Copy link
Member

Choose a reason for hiding this comment

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

I would also rather keep it the current way if there is only a one-second speedup...

tesuji added a commit to tesuji/rust-clippy that referenced this pull request Oct 22, 2019
@tesuji
enable some integration tests
953de1f 
The aid is to check if clippy got much slower over the last few days.
But not because of rust-lang#4703.
@tesuji tesuji mentioned this pull request Oct 22, 2019
Closed
tesuji added a commit to tesuji/rust-clippy that referenced this pull request Oct 23, 2019
@tesuji
enable some integration tests
cef7048 
The aid is to check if clippy got much slower over the last few days.
But not because of rust-lang#4703.
@tesuji
Copy link
Contributor Author

tesuji commented Oct 23, 2019

I would close this PR.
I close it not because I feel that using archive is neglectable 1st faster using git.
But there are regression in clippy run-time in integration tests that I cannot resolve.
I tried to resolve it in #4712 by:

  • using git again and measure run-time
  • using git again, remove .git dir and measure run-time

But none of them causes regressions in run-time like this PR.
Unless I could resolve that regression, I close this PR.

@tesuji tesuji closed this Oct 23, 2019
@tesuji tesuji deleted the donot-clone branch October 23, 2019 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthiaskrgr matthiaskrgr matthiaskrgr left review comments

@flip1995 flip1995 flip1995 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@tesuji @phansch @bors @matthiaskrgr @flip1995