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

ci: use incremental build in ci #10053

Merged
merged 3 commits into from Apr 7, 2021
Merged

Conversation

kt3k
Copy link
Member

@kt3k kt3k commented Apr 7, 2021

This PR enables the incremental build of rust code in CI by the following changes:

  • Enables it by env var CARGO_INCREMENTAL=1. (release profile has incremental = false setting by default.)
  • Uses matrix.kind in part of cache key because test_release and test_debug can't share the cache.
  • Restores each source file's timestamp based on git log. ( ./tools/restore_mtime.py )
    • git doesn't store timestamp of files, and cargo triggers rebuild of source file based on timestamps. So we need to preserve timestamp of each file somehow. I found this script (this script restores the timestamp of files based on commit log), and it seems working.

I tested this setting in my detached fork. windows job passed in 11 minutes https://github.com/kt3k/deno_ci_test/runs/2285441107

@@ -58,7 +58,7 @@ jobs:
# Use depth > 1, because sometimes we need to rebuild main and if
# other commits have landed it will become impossible to rebuild if
# the checkout is too shallow.
fetch-depth: 5
fetch-depth: '0'
Copy link
Member

Choose a reason for hiding this comment

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

I believe this comment still stands - maybe we shouldn't go with depth 5 but 1 or 2 is highly desired

Copy link
Member Author

@kt3k kt3k Apr 7, 2021

Choose a reason for hiding this comment

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

0 means disabling shallow clone i.e. fetching all history. (this is needed for updating the modified timestamps based on git history) ref: https://github.com/actions/checkout#checkout-v2

So I'll update the comment.

@piscisaureus
Copy link
Member

I think you should also do this for submodules. Currently only files from the "root" git repo get treated.

@piscisaureus
Copy link
Member

We can think of ways to make this even better, but let's land it as-is first and enjoy the 2x reduction in CI build time immediately.

@piscisaureus
Copy link
Member

@bartlomieju You have the final say.

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

We can think of ways to make this even better, but let's land it as-is first and enjoy the 2x reduction in CI build time immediately.

Let's go 🚀

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

LGTM - thank you Yoshiya!

@ry ry merged commit fd65e6d into denoland:main Apr 7, 2021
@kt3k kt3k deleted the try-incremental-build branch April 7, 2021 14:43
piscisaureus added a commit to piscisaureus/deno that referenced this pull request Apr 10, 2021
piscisaureus added a commit to piscisaureus/deno that referenced this pull request Apr 10, 2021
piscisaureus added a commit to piscisaureus/deno that referenced this pull request Apr 10, 2021
piscisaureus added a commit to piscisaureus/deno that referenced this pull request Apr 11, 2021
Sign up for free to 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.

None yet

4 participants