-
Notifications
You must be signed in to change notification settings - Fork 6k
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
Add a "pub workspace" to the root of the engine repository #53539
Changes from 30 commits
54d1ca6
2068ba1
fcde54c
50fa853
144ec4a
d78b88a
9f49e49
ff2aa36
7812873
a467fb6
901b47f
4f99625
d1ba509
b4c4186
9485af8
2638753
627d84c
46853e1
339abd3
3ed2f04
d889220
2ba6436
eb2303a
ef6605d
2d727a6
8ee0c85
d2aa032
d1a92b4
d4fa1df
7703083
e0c31a8
784b9d8
77f04a8
da0d118
9ee12d0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,173 @@ | ||
# This file represents a "workspace" that applies to the whole repository. | ||
# | ||
# See <https://flutter.dev/go/pub-workspace> for details. | ||
# | ||
# The `flutter/engine` repository is a quasi-monorepo, with multiple Dart tools | ||
# and packages that are all interdependent. Third party dependencies are managed | ||
# by the `DEPS` file in the root of the repository, and are synced to either the | ||
# `third_party` (i.e. `//flutter/third_party`) or `../third_party` (i.e. | ||
# `//third_party`) directories by `gclient sync`. | ||
# | ||
# Every dependency declared here are dependencies used by _one or more_ of the | ||
# packages in the repository (though there is no enforcement of this). This file | ||
# then generates a `.dart_tool/package_config.json` file that is used by the | ||
# rest of the repository to resolve dependencies. | ||
# | ||
# ============================================================================== | ||
# WORKFLOWS | ||
# ============================================================================== | ||
# | ||
# ------------------------------------------------------------------------------ | ||
# (1) ADDING A NEW DEPENDENCY | ||
# ------------------------------------------------------------------------------ | ||
# Dependencies need to be added either via the DEPS file, or by checking if they | ||
# are available in the vendored Dart SDK (see notes below on library locations). | ||
# If dependencies are available, see (4) for how to add dependencies to a package in the workspace. | ||
# | ||
# ------------------------------------------------------------------------------ | ||
# (2) CREATING A NEW PACKAGE | ||
# ------------------------------------------------------------------------------ | ||
# If creating a package, say in ./tools or ./tools/pkg, ensure the following | ||
# header in its respective `pubspec.yaml`: | ||
# ``` | ||
# # We don't publish packages to pub.dev from the engine repository. | ||
# publish_to: none | ||
# | ||
# # Required for workspace support. | ||
# environment: | ||
# sdk: ^3.5.0-294.0.dev | ||
# | ||
# # This package is managed as part of the engine workspace. | ||
# resolution: workspace | ||
# ``` | ||
# | ||
# See (4) for how to add dependencies to a package in the workspace. | ||
# | ||
# ------------------------------------------------------------------------------ | ||
# (3) MIGRATING A NON-WORKSPACE PACKAGE TO USING THE WORKSPACE | ||
# ------------------------------------------------------------------------------ | ||
# Many packages in this repo are still using a pre-workspace style pubspec.yaml, | ||
# either with manually declared `dependency_overrides` (much of ./tools) or by | ||
# using pub (./web_sdk, ./lib/web_ui). To migrate a package to the workspace: | ||
# | ||
# A. Add the `resolution: workspace` field to the pubspec.yaml. | ||
# B. Update the minimum SDK version to at least `^3.5.0-294.0.dev`. | ||
# C. Add the package to the `workspace` field in this file. | ||
# D. Ensure every dependency in the package is added to the `dependencies` field | ||
# in this file, following instructions in (4). | ||
# | ||
# Once `dart pub get` is run on the workspace, the package will be resolved as | ||
# part of the workspace, and the `dependency_overrides` in this file will be | ||
# applied to the package. | ||
# | ||
# ------------------------------------------------------------------------------ | ||
# (4) ADDING DEPENDENCIES TO A PACKAGE IN THIS WORKSPACE | ||
# ------------------------------------------------------------------------------ | ||
# When adding a dependency to a package in the workspace, add the dependency to | ||
# the `dependencies` field in this file. If the dependency is located within | ||
# the repository, use the `path` field to point to the package. | ||
# | ||
# If the dependency is a third party package, add it to the | ||
# `dependency_overrides` field in this file. The `any` version constraint is | ||
# used to indicate that the version of the package is not important, as it is | ||
# managed by the `DEPS` file. | ||
|
||
name: _engine_workspace | ||
|
||
# Required for workspace support. | ||
environment: | ||
sdk: ^3.5.0-294.0.dev | ||
|
||
# Declare all packages that are part of the workspace. | ||
workspace: | ||
- tools/engine_tool | ||
|
||
# Declare all dependencies that are used by one or more packages. | ||
# | ||
# A few notes: | ||
# 1. There is no distinction between "dependencies" and "dev_dependencies"; | ||
# those notions are for *publishing* packages, not for managing a workspace. | ||
# Specific packages in the workspace itself will declare whether they are | ||
# dependencies or dev_dependencies, but here it is a union of both. | ||
# | ||
# 2. The `any` version constraint is used to indicate that the version of the | ||
# package is not important, as it is managed by the `DEPS` file. In other | ||
# words, "if the test pass, ship it". | ||
# | ||
# While not enforced by tooling, try to keep this list in alphabetical order. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You could enable: https://dart.dev/tools/linter-rules/sort_pub_dependencies There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a TODO for now, thanks! |
||
# TODO(matanlurey): https://dart.dev/tools/linter-rules/sort_pub_dependencies. | ||
dependencies: | ||
args: any | ||
async_helper: any | ||
expect: any | ||
file: any | ||
logging: any | ||
meta: any | ||
path: any | ||
platform: any | ||
process_runner: any | ||
smith: any | ||
|
||
# Instructs pub on how to resolve the dependencies that are part of "DEPS". | ||
# | ||
# For historic reasons, there are ~3 or so places packages might be located: | ||
# | ||
# - `./third_party/pkg/{name}`: for packages vended directly as part of "DEPS". | ||
# Usually these are Flutter engine specific packages, i.e. they did not exist | ||
# in the Dart vended SDK (the other options below). Typically these originate | ||
# from pub (https://pub.dev) and are mirrored into a Google Git repository: | ||
# <https://flutter.googlesource.com/>. | ||
# | ||
# - `./third_party/dart/pkg/{name}`: for packages that lives *in* the Dart SDK, | ||
# which is in turn vendored into the Flutter engine repository. You can see | ||
# a full list of available packages here: | ||
# <https://github.com/dart-lang/sdk/tree/main/pkg>. | ||
# | ||
# - `./third_party/dart/third_party/pkg/{name}`: for packages that are vendored | ||
# into the Dart SDK from pub.dev. These are often first-party packages from | ||
# the Dart team, but not part of the Dart SDK itself. You can see a full list | ||
# of available packages here: | ||
# <https://github.com/dart-lang/sdk/blob/main/DEPS>. | ||
dependency_overrides: | ||
jtmcdole marked this conversation as resolved.
Show resolved
Hide resolved
|
||
args: | ||
path: ./third_party/dart/third_party/pkg/args | ||
async: | ||
path: ./third_party/dart/third_party/pkg/async | ||
async_helper: | ||
path: ./third_party/dart/pkg/async_helper | ||
collection: | ||
path: ./third_party/dart/third_party/pkg/collection | ||
engine_build_configs: | ||
path: ./tools/pkg/engine_build_configs | ||
engine_repo_tools: | ||
path: ./tools/pkg/engine_repo_tools | ||
expect: | ||
path: ./third_party/dart/pkg/expect | ||
file: | ||
path: ./third_party/dart/third_party/pkg/file/packages/file | ||
litetest: | ||
path: ./testing/litetest | ||
logging: | ||
path: ./third_party/dart/third_party/pkg/logging | ||
meta: | ||
path: ./third_party/dart/pkg/meta | ||
path: | ||
path: ./third_party/dart/third_party/pkg/path | ||
platform: | ||
path: ./third_party/pkg/platform | ||
process: | ||
path: ./third_party/pkg/process | ||
process_fakes: | ||
path: ./tools/pkg/process_fakes | ||
process_runner: | ||
path: ./third_party/pkg/process_runner | ||
smith: | ||
path: ./third_party/dart/pkg/smith | ||
source_span: | ||
path: ./third_party/dart/third_party/pkg/source_span | ||
string_scanner: | ||
path: ./third_party/dart/third_party/pkg/string_scanner | ||
term_glyph: | ||
path: ./third_party/dart/third_party/pkg/term_glyph | ||
yaml: | ||
path: ./third_party/dart/third_party/pkg/yaml |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,10 +14,15 @@ | |
import subprocess | ||
import sys | ||
|
||
THIS_DIR = os.path.abspath(os.path.dirname(__file__)) | ||
sys.path += [os.path.join(THIS_DIR, '..', 'third_party', 'pyyaml', 'lib3')] | ||
import yaml # type: ignore | ||
|
||
SRC_ROOT = os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) | ||
ENGINE_DIR = os.path.join(SRC_ROOT, 'flutter') | ||
|
||
ALL_PACKAGES = [ | ||
os.path.join(ENGINE_DIR), | ||
os.path.join(ENGINE_DIR, 'ci'), | ||
os.path.join(ENGINE_DIR, 'flutter_frontend_server'), | ||
os.path.join(ENGINE_DIR, 'impeller', 'tessellator', 'dart'), | ||
|
@@ -64,7 +69,14 @@ def fetch_package(pub, package): | |
return 0 | ||
|
||
|
||
def check_package(package): | ||
def package_uses_workspace_resolution(package): | ||
pubspec = os.path.join(package, 'pubspec.yaml') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see pyyaml in the DEPS. Is that not usable? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good, I used the same pattern as #!/usr/bin/env vpython3
# [VPYTHON:BEGIN]
# python_version: "3.8"
# wheel <
# name: "infra/python/wheels/pyyaml/${platform}_${py_python}_${py_abi}"
# version: "version:5.4.1.chromium.1"
# >
# [VPYTHON:END] There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should only be fetching dependencies from the network from the DEPS file. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, pubspec files with This script will throw saying "no package_config.json", hence this change. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, sorry. My comment was in reference to the discussion of pyyaml and using the wheel. |
||
|
||
with open(pubspec) as pubspec_file: | ||
return yaml.safe_load(pubspec_file).get('resolution') == 'workspace' | ||
|
||
|
||
def check_package_config(package): | ||
package_config = os.path.join(package, '.dart_tool', 'package_config.json') | ||
pub_count = 0 | ||
with open(package_config) as config_file: | ||
|
@@ -119,6 +131,9 @@ def find_unlisted_packages(): | |
|
||
|
||
def main(): | ||
# Intentionally use the Dart SDK prebuilt instead of the Flutter prebuilt | ||
# (i.e. prebuilts/{platform}/dart-sdk/bin/dart) because the script has to run | ||
# in a monorepo build *before* the newer Dart SDK has been built from source. | ||
dart_sdk_bin = os.path.join( | ||
SRC_ROOT, 'flutter', 'third_party', 'dart', 'tools', 'sdks', 'dart-sdk', 'bin' | ||
) | ||
|
@@ -139,7 +154,8 @@ def main(): | |
for package in ALL_PACKAGES: | ||
if fetch_package(pubcmd, package) != 0: | ||
return 1 | ||
pub_count = pub_count + check_package(package) | ||
if not package_uses_workspace_resolution(package): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For my understanding, is it possible for a pubspec that uses the workspace to add a dependency on a package that isn't in the workspace? Like can a workspace-using pubspec add a new dependency and pull a package from pub that isn't declared in the workspace? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm... I'm actually not sure. I assume no, but let me give it a shot. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Short answer: Yes, it's possible. It ends up adding it to the root Long answer: Our script to protect against pub dependencies still works, as it catches the root package config: diff --git a/tools/engine_tool/lib/main.dart b/tools/engine_tool/lib/main.dart
index e524e07ad0..d2864ee1db 100644
--- a/tools/engine_tool/lib/main.dart
+++ b/tools/engine_tool/lib/main.dart
@@ -5,6 +5,7 @@
import 'dart:ffi' as ffi show Abi;
import 'dart:io' as io show Directory, Platform, exitCode, stderr;
+import 'package:archive/archive.dart';
import 'package:engine_build_configs/engine_build_configs.dart';
import 'package:engine_repo_tools/engine_repo_tools.dart';
import 'package:path/path.dart' as p;
@@ -17,6 +18,7 @@ import 'src/logger.dart';
import 'src/phone_home.dart';
void main(List<String> args) async {
+ print(getAdler32);
if (phoneHome(args)) {
return;
}
diff --git a/tools/engine_tool/pubspec.yaml b/tools/engine_tool/pubspec.yaml
index 1b71b9b688..83759735d5 100644
--- a/tools/engine_tool/pubspec.yaml
+++ b/tools/engine_tool/pubspec.yaml
@@ -13,6 +13,7 @@ environment:
resolution: workspace
dependencies:
+ archive: any
args: any
engine_build_configs: any
engine_repo_tools: any ... and then: matanl@matanl-macbookpro4 engine_tool % gclient sync
Syncing projects: 100% (143/143), done.
Running hooks: 46% ( 7/15) pub get --offline
________ running 'python3 src/flutter/tools/pub_get_offline.py' in '/Users/matanl/Developer/engine'
Error: package "archive" was fetched from pub
Error: package "crypto" was fetched from pub
Error: package "typed_data" was fetched from pub
Error: 3 packages were fetched from pub for /Users/matanl/Developer/engine/src/flutter
Please fix the pubspec.yaml for /Users/matanl/Developer/engine/src/flutter so that all dependencies are path dependencies
Error: Command 'python3 src/flutter/tools/pub_get_offline.py' returned non-zero exit status 1 in /Users/matanl/Developer/engine
Error: package "archive" was fetched from pub
Error: package "crypto" was fetched from pub
Error: package "typed_data" was fetched from pub
Error: 3 packages were fetched from pub for /Users/matanl/Developer/engine/src/flutter
Please fix the pubspec.yaml for /Users/matanl/Developer/engine/src/flutter so that all dependencies are path dependencies I do think the UX of allowing package-specific dependencies and silently adding them to the root pubspec is confusing, but not a blocker for this effort. I'll file an issue on dart-lang/pub and see what the team thinks in terms of UX. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in dart-lang/pub#4328. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for checking! |
||
pub_count = pub_count + check_package_config(package) | ||
|
||
if pub_count > 0: | ||
return 1 | ||
|
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.
These comments are phenomenal, excellent work!