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

Only bundle assets and plugins from transitive closure of dependencies #160443

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

sigurdm
Copy link
Contributor

@sigurdm sigurdm commented Dec 17, 2024

Fixes dart-lang/pub#4486
Fixes dart-lang/pub#4473
Fixes #79261
Fixes #160142
Fixes #155242

Use the new .dart_tool/package_graph.json file to compute the dependency graph. Determining

  • which are transitive dependencies of the main app's direct and dev-dependencies, (not other packages from the workspace)
  • which of these are only in the transitive set of the direct dependencies

Bundles assets from dev_dependencies when running flutter test, but not otherwise.

@github-actions github-actions bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Dec 17, 2024
@bkonyi bkonyi requested review from andrewkolos and bkonyi December 19, 2024 21:28
@sigurdm sigurdm force-pushed the only_bundle_assets_from_dependencies branch from 1024789 to 95ce951 Compare December 20, 2024 09:15
@sigurdm sigurdm changed the title Only bundle assets from transitive closure of dependencies Only bundle assets and plugins from transitive closure of dependencies Dec 20, 2024
@github-actions github-actions bot added the a: desktop Running on desktop label Jan 9, 2025
@sigurdm
Copy link
Contributor Author

sigurdm commented Jan 10, 2025

I think all local tests are passing now. (perhaps we should refactor some tests, so package_config.json doesn't have to be created uniquely for so many tests....)

Not sure what happens in the customer tests, but I think it might be unrelated to this change.

Copy link
Contributor

@bkonyi bkonyi left a comment

Choose a reason for hiding this comment

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

Initial quick review. Wil give this another pass later.

@sigurdm sigurdm requested a review from bkonyi January 14, 2025 13:07
@sigurdm
Copy link
Contributor Author

sigurdm commented Jan 21, 2025

Gentle ping on this one

@gabrielgarciagava
Copy link
Contributor

@spydon I know you are one of the melos maintainers. I would suggest you to keep track of this pull request. I would say this is a blocker to release the workspaces support to melos.

@spydon
Copy link

spydon commented Jan 22, 2025

@spydon I know you are one of the melos maintainers. I would suggest you to keep track of this pull request. I would say this is a blocker to release the workspaces support to melos.

Thanks, I'm already subscribed to it. I've created one of the issues above. :)

Copy link
Contributor

@andrewkolos andrewkolos left a comment

Choose a reason for hiding this comment

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

LGTM with nits

@sigurdm
Copy link
Contributor Author

sigurdm commented Jan 24, 2025

Do you think you could help me out with the customer_testing tests?

I cannot understand how my change could cause errors like:

 00:11 +8: /b/s/w/ir/x/t/flutter_customer_testing.super_editor.UDMNRS/tests/super_text_layout/test/super_text_test.dart: SuperText RenderSuperTextLayout calculates line count for one line of text
| ══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞════════════════════════════════════════════════════
| The following TestFailure was thrown running a test:
| Expected: <1>
|   Actual: <2>
|
| When the exception was thrown, this was the stack:
| #4      main.<anonymous closure>.<anonymous closure>.<anonymous closure>.<anonymous closure> (file:///b/s/w/ir/x/t/flutter_customer_testing.super_editor.UDMNRS/tests/super_text_layout/test/super_text_test.dart:203:11)
| <asynchronous suspension>
| #5      testWidgets.<anonymous closure>.<anonymous closure> (package:flutter_test/src/widget_tester.dart:193:15)
| <asynchronous suspension>
| #6      TestWidgetsFlutterBinding._runTestBody (package:flutter_test/src/binding.dart:1064:5)
| <asynchronous suspension>
| <asynchronous suspension>
| (elided one frame from package:stack_trace)

But also I don't want to land this with red tests I don't understand...

@andrewkolos
Copy link
Contributor

From what I can tell, the tests depend on the Roboto font being available (src). Before, it was getting bundled from the golden_toolkit package.

To verify yourself, clone the super_editor repo and navigate to the failing test (https://github.com/superlistapp/super_editor/blob/dc7013f2f9be145d655e891eb2502baf2d6795a1/super_text_layout/test/super_text_test.dart#L192). To see what assets are getting bundled within the test code, add

final assets = (await AssetManifest.loadFromAssetBundle(rootBundle))
  .listAssets();
print(assets.join('\n'));

When on master, I see

packages/golden_toolkit/fonts/Roboto-Regular.ttf

which I don't see when I have this PR checked out.

@sigurdm
Copy link
Contributor Author

sigurdm commented Jan 28, 2025

I understand now, thanks!

The super_text_layout package dev_depends on golden_toolkit which has the font as an asset. This PR does not allow you to use assets from your dev_dependencies.

So in general when testing, you might want to use assets from your dev_dependencies, but not when building for deployment I had not really realized that.

I see three ways forward:

  1. We teach flutter_tool to bundle different assets when testing?
    I wonder if this is realistic? Not sure exactly what this would require. @andrewkolos @bkonyi
  2. We could also revert that part of this PR, so that you still bundle all assets from dev_dependencies (would no longer solve package assets declared under dev_dependency shouldn't be included in release bundle #79261)
  3. We could also require all assets to come from dependencies, and update the customer test to have golden_toolkit in its dependencies

@andrewkolos
Copy link
Contributor

So in general when testing, you might want to use assets from your dev_dependencies, but not when building for deployment I had not really realized that.

Yeah, I was thinking the same. There are probably edge cases where including a font from a dev dependency would unexpectedly cause a test to pass (i.e. the font was meant to be included from a regular dependency or the assets section in your pubspec); but I expect such a case would be extremely rare. That is to say, I think it is fine to continue to include assets from dev dependencies in tests—at least for now.

@bkonyi
Copy link
Contributor

bkonyi commented Jan 30, 2025

I think including assets from dev dependencies in tests is fine since I assume our main concern here is reducing bundle size in shipped applications.

@sigurdm
Copy link
Contributor Author

sigurdm commented Jan 31, 2025

Attached g3fix https://critique.corp.google.com/cl/721374511

@bkonyi
Copy link
Contributor

bkonyi commented Jan 31, 2025

Thanks for addressing the comments! LGTM!

@sigurdm
Copy link
Contributor Author

sigurdm commented Feb 3, 2025

Seems #161826 and #161343 are interferring with this.

@camsim99 do you know the desired semantics around plugins and assets from dev-dependencies? Should they be included in debug builds but not in release? Or should they be included for testing only (or not at all)?

@camsim99
Copy link
Contributor

camsim99 commented Feb 3, 2025

Seems #161826 and #161343 are interferring with this.

@camsim99 do you know the desired semantics around plugins and assets from dev-dependencies? Should they be included in debug builds but not in release? Or should they be included for testing only (or not at all)?

My understanding is that we are working towards what I think #79261 proposes, i.e. assets from dev dependencies should be included in debug builds but not release. The changes I made are irrespective to testing I think. cc @matanlurey for input

Copy link
Contributor

@matanlurey matanlurey left a comment

Choose a reason for hiding this comment

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

So the reason this failing is because the feature that @camsim99 and I implemented, which is, to omit plugins specified in dev_dependencies from release mode apps, looks like was broken as a result of the refactoring in this PR.

I can try cloning it locally and debug a bit tomorrow morning if you are still stuck, but as a starting point:

Not directly related: It's nice that this code was refactored to be synchronous and read the file system instead of shelling out to pub deps, but this was a large refactoring and in the future I should be included as a PR reviewer as someone who has to also maintain and understand these changes.

Copy link
Contributor

@matanlurey matanlurey left a comment

Choose a reason for hiding this comment

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

Had a chance to do a longer review. Still needs more work, and I still think it makes sense to break this up into 2+ PRs.

@github-actions github-actions bot removed platform-ios iOS applications specifically framework flutter/packages/flutter repository. See also f: labels. engine flutter/engine repository. See also e: labels. a: animation Animation APIs f: material design flutter/packages/flutter/material repository. a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) platform-fuchsia Fuchsia code specifically f: scrolling Viewports, list views, slivers, etc. f: cupertino flutter/packages/flutter/cupertino repository platform-windows Building on or for Windows specifically d: examples Sample code and demos f: gestures flutter/packages/flutter/gestures repository. platform-web Web applications specifically platform-linux Building on or for Linux specifically e: impeller Impeller rendering backend issues and features requests team-tool Owned by Flutter Tool team d: docs/ flutter/flutter/docs, for contributors platform-macos labels Mar 6, 2025
@jmagman jmagman requested review from vashworth and removed request for vashworth March 7, 2025 18:48
@loic-sharma loic-sharma self-requested a review March 7, 2025 18:56
@@ -24,8 +24,8 @@ Future<PackageConfig> currentPackageConfig() async {
// TODO(sigurdm): Only call this once per run - and read in from BuildInfo.
File? findPackageConfigFile(Directory dir) {
final FileSystem fileSystem = dir.fileSystem;
Directory candidateDir = fileSystem.directory(fileSystem.path.normalize(dir.absolute.path));
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 is extracted in #165392

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: desktop Running on desktop team-ios Owned by iOS platform team tool Affects the "flutter" command-line tool. See also t: labels.
Projects
None yet
7 participants