-
Notifications
You must be signed in to change notification settings - Fork 28.2k
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
base: master
Are you sure you want to change the base?
Only bundle assets and plugins from transitive closure of dependencies #160443
Conversation
1024789
to
95ce951
Compare
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. |
There was a problem hiding this 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.
Gentle ping on this one |
@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. :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with nits
packages/flutter_tools/test/general.shard/web/compile_web_test.dart
Outdated
Show resolved
Hide resolved
packages/flutter_tools/test/general.shard/compute_dev_dependencies_test.dart
Outdated
Show resolved
Hide resolved
packages/flutter_tools/test/general.shard/compute_dev_dependencies_test.dart
Outdated
Show resolved
Hide resolved
Do you think you could help me out with the customer_testing tests? I cannot understand how my change could cause errors like:
But also I don't want to land this with red tests I don't understand... |
From what I can tell, the tests depend on the Roboto font being available (src). Before, it was getting bundled from the 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
which I don't see when I have this PR checked out. |
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:
|
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 |
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. |
Attached g3fix https://critique.corp.google.com/cl/721374511 |
Thanks for addressing the comments! LGTM! |
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 |
There was a problem hiding this 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:
- https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8723951633781029105/+/u/run_test.dart_for_tool_tests_shard_and_subshard_general/stdout "plugins injectPlugins in release mode excludes dev dependencies from MacOS plugin registrant"
- https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8723951633490693953/+/u/run_android_release_builds_exclude_dev_dependencies_test/stdout "android_release_builds_exclude_dev_dependencies_test"
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.
There was a problem hiding this 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.
packages/flutter_tools/test/general.shard/compute_dev_dependencies_test.dart
Outdated
Show resolved
Hide resolved
packages/flutter_tools/test/general.shard/compute_dev_dependencies_test.dart
Outdated
Show resolved
Hide resolved
packages/flutter_tools/test/general.shard/compute_dev_dependencies_test.dart
Outdated
Show resolved
Hide resolved
packages/flutter_tools/test/general.shard/compute_dev_dependencies_test.dart
Outdated
Show resolved
Hide resolved
@@ -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)); |
There was a problem hiding this comment.
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
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. DeterminingBundles assets from dev_dependencies when running
flutter test
, but not otherwise.