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

build(bazel): improve remote caching for AIO local deps build #48579

Closed
wants to merge 1 commit into from
Closed

build(bazel): improve remote caching for AIO local deps build #48579

wants to merge 1 commit into from

Conversation

kormide
Copy link
Contributor

@kormide kormide commented Dec 22, 2022 •
edited
Loading

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Architect tests for the AIO local deps build run on every pull request.

What is the new behavior?

Test results are cached properly, significantly speeding up the build when there are no AIO changes.

Does this PR introduce a breaking change?

  • Yes
  • No

@kormide kormide force-pushed the cache-miss-testing-2 branch 26 times, most recently from 5c1bbcb to 7645553 Compare December 25, 2022 21:21
@angular-robot angular-robot bot added the area: build & ci Related the build and CI infrastructure of the project label Dec 25, 2022
@ngbot ngbot bot added this to the Backlog milestone Dec 25, 2022
@kormide kormide changed the title bazel cache miss testing build(bazel): improve remote caching for AIO local deps build Dec 25, 2022
@kormide kormide force-pushed the cache-miss-testing-2 branch from 7645553 to dd13eb3 Compare December 25, 2022 21:23
@kormide kormide force-pushed the cache-miss-testing-2 branch from dd13eb3 to f410824 Compare December 25, 2022 21:25
kormide
kormide commented Dec 25, 2022
View reviewed changes
aio/BUILD.bazel
@@ -125,7 +125,6 @@ copy_to_directory(
# All source and configuration files required to build the docs app
APPLICATION_FILES = [
"angular.json",
"package.json",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This wasn't a necessary input to the architect actions.

@kormide kormide force-pushed the cache-miss-testing-2 branch 2 times, most recently from 2f9f5ae to be28415 Compare December 25, 2022 22:02
@kormide kormide requested a review from devversion December 25, 2022 22:14
@kormide kormide marked this pull request as ready for review December 25, 2022 22:14
@pullapprove pullapprove bot requested a review from AndrewKushnir December 25, 2022 22:14
@kormide
build(bazel): improve remote caching for AIO local deps build
62e7ceb 
Fix non-hermetic zipping of example zips by fixing the zip entry timestamps.

I also hardcoded stamp values in stable-status.txt and volatile-status.txt using the workspace status command for the aio_local_deps config to improve cache performance. The Bazel remote cache appears to not ignore volatile-status.txt when there are no other changes, unlike the local Bazel cache:

bazelbuild/bazel#10075 (comment)
@kormide kormide force-pushed the cache-miss-testing-2 branch from be28415 to 62e7ceb Compare December 25, 2022 22:16
@kormide
Copy link
Contributor Author

kormide commented Dec 26, 2022 •
edited
Loading

Note that the architect build (//aio:build-app) is also not hermetic as the ngsw.json file it generates has a timestamp in it. I'm not sure if it's correct to remove or hardcode that timestamp, and if it's worth the effort to do that.

devversion
devversion approved these changes Dec 30, 2022
View reviewed changes
Copy link
Member

@devversion devversion left a comment

Choose a reason for hiding this comment

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

Nice, thanks!

devversion
devversion reviewed Dec 30, 2022
View reviewed changes
aio/scripts/local-workspace-status.mjs
// and will invalidate actions that depend on it when the values change.
// https://github.com/bazelbuild/bazel/issues/10075#issuecomment-546872111
console.log(`\
BUILD_TIMESTAMP 0
Copy link
Member

Choose a reason for hiding this comment

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

One thing I found interesting from the Bazel issue. It sounds like we shouldn't really rely on stamping for development builds. This makes a lot of sense to me. I think this is a great improvement regardless and solves the issue, but I wonder if long-term Angular should make sure AIO can be built w/ local packages unstamped? I believe we could patch/configure architect appropriately

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that altering architect is probably the right long-term solution.

@devversion devversion added action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release labels Dec 30, 2022
@devversion
Copy link
Member

This PR was merged into the repository by commit 0ae80a7.

devversion pushed a commit that referenced this pull request Jan 2, 2023
@kormide @devversion
build(bazel): improve remote caching for AIO local deps build (#48579)
4ae6655 
Fix non-hermetic zipping of example zips by fixing the zip entry timestamps.

I also hardcoded stamp values in stable-status.txt and volatile-status.txt using the workspace status command for the aio_local_deps config to improve cache performance. The Bazel remote cache appears to not ignore volatile-status.txt when there are no other changes, unlike the local Bazel cache:

bazelbuild/bazel#10075 (comment)

PR Close #48579
@devversion devversion closed this in 0ae80a7 Jan 2, 2023
trekladyone pushed a commit to trekladyone/angular that referenced this pull request Feb 1, 2023
@kormide @trekladyone
build(bazel): improve remote caching for AIO local deps build (angula…
614d432 
…r#48579)

Fix non-hermetic zipping of example zips by fixing the zip entry timestamps.

I also hardcoded stamp values in stable-status.txt and volatile-status.txt using the workspace status command for the aio_local_deps config to improve cache performance. The Bazel remote cache appears to not ignore volatile-status.txt when there are no other changes, unlike the local Bazel cache:

bazelbuild/bazel#10075 (comment)

PR Close angular#48579
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Feb 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@devversion devversion devversion approved these changes

@AndrewKushnir AndrewKushnir Awaiting requested review from AndrewKushnir

@josephperrott josephperrott Awaiting requested review from josephperrott

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: build & ci Related the build and CI infrastructure of the project target: patch This PR is targeted for the next patch release
Projects
None yet
Milestone
Backlog
Development

Successfully merging this pull request may close these issues.
2 participants
@kormide @devversion