Skip to content

Fix dep tracking - #147709

Merged
auto-submit[bot] merged 3 commits into
masterfrom
fix-dep-tracking
May 7, 2024
Merged

Fix dep tracking#147709
auto-submit[bot] merged 3 commits into
masterfrom
fix-dep-tracking

Conversation

@dcharkes

@dcharkes dcharkes commented May 2, 2024

Copy link
Copy Markdown
Contributor

Fix dependency tracking in the build.

Testing

It's not clear to me where to test the caching behavior of specific targets.

  • test/general.shard/build_system/targets/common_test.dart
    • This doesn't test the caching behavior of any targets
  • test/general.shard/build_system/build_system_test.dart
  • test/general.shard/cache_test.dart
    • Both of these don't test specific Targets, these have TestTargets.
  • test/integration.shard/isolated/native_assets_test.dart
    • This could work, but it's an integration test that already takes long to run.

Pre-launch Checklist

@github-actions github-actions Bot added the tool Affects the "flutter" command-line tool. See also t: labels. label May 2, 2024
@dcharkes
dcharkes requested a review from christopherfujino May 2, 2024 11:06
@dcharkes

dcharkes commented May 2, 2024

Copy link
Copy Markdown
Contributor Author

@christopherfujino Any suggestions on how to test this? I could add unit tests to the Target that they report inputs and outputs, but that doesn't actually test if that will be picked up by the build system.

Also, I'm slightly surprised that dependencies is not enough for the caching. I guess it makes sense that inputs and outputs is enough. But then why do we have dependencies.

@dcharkes dcharkes mentioned this pull request May 2, 2024
Closed
@andrewkolos
andrewkolos requested review from andrewkolos and removed request for christopherfujino May 2, 2024 20:22
@knopp

knopp commented May 3, 2024

Copy link
Copy Markdown
Member

I can confirm this resolves the issue.

@craiglabenz craiglabenz mentioned this pull request May 6, 2024
Closed
@timmaffett

timmaffett commented May 6, 2024

Copy link
Copy Markdown
Contributor

I can confirm that this also fixes #147752

@dcharkes

dcharkes commented May 7, 2024

Copy link
Copy Markdown
Contributor Author

Hi @andrewkolos any chance you can get the review done? Multiple users are hit by this on the master branch.

@andrewkolos

andrewkolos commented May 7, 2024

Copy link
Copy Markdown
Contributor

Ah sorry, this slipped off my radar. I'll take a look today.

andrewkolos
andrewkolos approved these changes May 7, 2024
View reviewed changes

@andrewkolos andrewkolos left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am willing to LGTM now since this solves a pretty significant issue on master, though I do not have any additional insights on how to better test this onhand (outside of full integration test).

@dcharkes dcharkes added the autosubmit Merge PR when tree becomes green via auto submit App label May 7, 2024
@auto-submit
auto-submit Bot merged commit 6967ae5 into master May 7, 2024
@auto-submit
auto-submit Bot deleted the fix-dep-tracking branch May 7, 2024 17:44
@dcharkes

dcharkes commented May 7, 2024

Copy link
Copy Markdown
Contributor Author

Thanks @andrewkolos!

If you come up with a good way to test this, let me know.

@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 8, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 8, 2024
@engine-flutter-autoroll
6967ae5 Fix dep tracking (flutter/flutter#147709)
ff7b690
@zanderso zanderso mentioned this pull request May 8, 2024
Closed
@zanderso

zanderso commented May 8, 2024

Copy link
Copy Markdown
Member

This change has caused a somewhat large regression in build times #147982

@dcharkes

dcharkes commented May 8, 2024

Copy link
Copy Markdown
Contributor Author

Looking at the performance graph, it seems that the performance regression is the inverse of the performance gains by the PR that broke the dependency tracking (and made builds faster but broken).

@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 8, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 8, 2024
@engine-flutter-autoroll
6967ae5 Fix dep tracking (flutter/flutter#147709)
a89e73a
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 8, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 8, 2024
@engine-flutter-autoroll
6967ae5 Fix dep tracking (flutter/flutter#147709)
01c2682
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 8, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 8, 2024
@engine-flutter-autoroll
6967ae5 Fix dep tracking (flutter/flutter#147709)
11fc372
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 6, 2024
@engine-flutter-autoroll
6967ae5 Fix dep tracking (flutter/flutter#147709)
dc69291
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andrewkolos andrewkolos andrewkolos approved these changes

Assignees

No one assigned

Labels

autosubmit Merge PR when tree becomes green via auto submit App tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@dcharkes @knopp @timmaffett @andrewkolos @zanderso