[infra] Use Pub Workspaces
Closes #1223
- Update all packages to require Dart 3.6 or higher
- Use Pub workspaces for all packages
TODO:
- [x] Finish remaining packages
- [x] Verify that
native_assets_builder/test_data/*still works withdart pub get - [x] Verify that
native_assets_cli/example/*still works withdart pub get - [x] Verify that
dart analyzepasses at the workspace level` - [x] Bump CI to use Dart 3.6 for all packages (stable/beta, migrated dev -> stable)
- [x] Consolidate
analysis_options.yamlinto workspace - [ ] Verify tests still run (waiting for CI)
- [x] Update changelogs and pubspec versions (Is this needed?)
Leaving these packages unmigrated:
jnigen/android_test_runnernative_assets_builder/test_data/native_add_version_skew
Blocked by:
- #1905
package:firehoseneeds to usepackage:dart_apitool: ^0.20.2
- [x] I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:
- See our contributor guide for general expectations for PRs.
- Larger or significant changes should be discussed in an issue before creating a PR.
- Contributions to our repos should follow the Dart style guide and use
dart format. - Most changes should add an entry to the changelog and may need to rev the pubspec package version.
- Changes to packages require corresponding tests.
Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.
@dcharkes The jni package contains the following:
## Pin ffigen version because we are depending on internal APIs.
ffigen: 8.0.2
But the current ffigen version is 16.1.0-wip, and that's causing conflicts. In general, because it's pinning a specific version, even if I update it to 16.0.0, it will fail on 16.0.1. Does that mean jni should be excluded from the workspace, or should ffigen start exposing those APIs and keep them stable?
Edit: I bumped the dependency in jni and used the workspace version, and updated renamed members, and it passes analysis now
Possibly silly question, but does this require a pubspec bump and changelog addition?
I tried running jni/tools/generate_ffi_bindings.dart because I thought I broke it. Here's the output:
$ dart tool\generate_ffi_bindings.dart
INFO: Generating C wrappers
Unknown type ffi.UnsignedInt for return type
I tracked that down to these lines in jni/tool/wrapper_generator/generate_c_extensions.dart:
String jValueGetterOf(String returnType) {
const getters = {
'jboolean': 'z',
'jbyte': 'b',
'jshort': 's',
'jchar': 'c',
'jint': 'i',
'jsize': 'i', // jsize is an alias to jint
'jfloat': 'f',
'jlong': 'j',
'jdouble': 'd',
'jobject': 'l',
'jweak': 'l',
'jarray': 'l',
'jstring': 'l',
'jthrowable': 'l',
};
if (!getters.containsKey(returnType)) {
stderr.writeln('Unknown type $returnType for return type');
exit(1);
}
return getters[returnType]!;
}
Seems it's expecting some Java-y value and is getting a C-ish value instead. Any idea why this is happening? And should I be fixing it in this PR? I'm concerned it happened due to changing the version of ffigen from 8.0.2 to 16.0.0.
Unrelated, just wanted to flag the following test:
// pkgs\ffigen\test\collision_tests\decl_decl_collision_test.dart
/// Conflicts with ffi library prefix, name of prefix is changed.
Struct(name: 'ffi'),
This seems like it should result in a line like
import "dart:ffi" as _ffi;
but it doesn't -- both the struct and the import prefix are called ffi, and this causes analysis issues. I'd imagine this can be an issue in production code, but I'm also seeing the _expected file clearly shows it expects the generated code to use as ffi and have all those errors. Is the test incorrect?
I also consolidated all of the analysis_options.yaml files into one, in the workspace. While having a separate for each package makes the excludes sections cleaner and more self-contained, https://github.com/dart-lang/sdk/issues/55281#issuecomment-2569134294 indicates that there are significant performance benefits to using one file.
As part of merging the analysis_options.yaml, I also merged their options. So for example, ffigen has some rules that ffi does not. Since there were only a few of these rules, I enabled them for all repos. Running dart fix --apply and dart format on those new rules to packages that weren't using them before led to a larger diff, but there should be no semantic changes in any of my commits labeled Added X to analysis_options.yaml or Fixed lints.
(It might be easier to review this PR by commits instead of by files, if you weren't doing that already)
PR Health
Changelog Entry :heavy_check_mark:
| Package | Changed Files |
|---|
Changes to files need to be accounted for in their respective changelogs.
License Headers :heavy_check_mark:
// Copyright (c) 2025, the Dart project authors. Please see the AUTHORS file
// for details. All rights reserved. Use of this source code is governed by a
// BSD-style license that can be found in the LICENSE file.
| Files |
|---|
| no missing headers |
All source files should start with a license header.
Unrelated files missing license headers
| Files |
|---|
| pkgs/jni/lib/src/third_party/generated_bindings.dart |
| pkgs/objective_c/lib/src/ns_input_stream.dart |
coverage: 83.95% (-4.6%) from 88.557% when pulling bf5d11cc23b7879708aa16e4222531ccb7929b4e on Levi-Lesches:workspaces into 3ed5d3d07246cd792693584c2c42216739a634b7 on dart-lang:main.
🙏 !
I also consolidated all of the
analysis_options.yamlfiles into one, in the workspace. While having a separate for each package makes theexcludessections cleaner and more self-contained, dart-lang/sdk#55281 (comment) indicates that there are significant performance benefits to using one file.As part of merging the
analysis_options.yaml, I also merged their options. So for example,ffigenhas some rules thatffidoes not. Since there were only a few of these rules, I enabled them for all repos. Runningdart fix --applyanddart formaton those new rules to packages that weren't using them before led to a larger diff, but there should be no semantic changes in any of my commits labeledAdded X to analysis_options.yamlorFixed lints.
SGTM!
Possibly silly question, but does this require a pubspec bump and changelog addition?
pubspec bump: I don't believe so, users on an older SDK will not have pub resolve to newer versions of the packages.
changelog: yes, bumping minimum SDK constraint should be in the changelog. (Also, soon I'd like to bump things to 3.7 when it's out, because I want the new formatter.)
(It might be easier to review this PR by commits instead of by files, if you weren't doing that already)
Let's see how easy it is to get the whole PR green including the CI. Otherwise we can land the bumping the SDK versions, dependencies, and lints in separate PRs.
Verify tests still run (waiting for CI)
The workflows will likely need a bunch of updates.
- Every
pub getcommand needs to beflutterinstead ofdart? pub getcommands for all individual example and test_data directories should no longer be needed (except for the ones that are excluded from the workspace due to pinned deps)
@HosseinYousefi @liamappelbe PTAL.
Every pub get command needs to be flutter instead of dart?
It should be enough to download Flutter instead of Dart, then running the dart executable from there.
How do you want to handle all the logic around the dev channel? That doesn't exist in Flutter. I can use beta, but would that really solve the issue? Or would these tests need to work on Dart before the Flutter team picks that SDK version for their next beta? For now, I switched the CI to Flutter, so let's see if it goes green this time.
Unrelated, just wanted to flag the following test:
// pkgs\ffigen\test\collision_tests\decl_decl_collision_test.dart /// Conflicts with ffi library prefix, name of prefix is changed. Struct(name: 'ffi'),This seems like it should result in a line like
import "dart:ffi" as _ffi;but it doesn't -- both the struct and the import prefix are called
ffi, and this causes analysis issues. I'd imagine this can be an issue in production code, but I'm also seeing the_expectedfile clearly shows it expects the generated code to useas ffiand have all those errors. Is the test incorrect?
Looking at the blame for that file, this test has been broken for over a year. I'll need to fix it later. Filed a bug: https://github.com/dart-lang/native/issues/1889
Every pub get command needs to be flutter instead of dart?
It should be enough to download Flutter instead of Dart, then running the
dartexecutable from there.How do you want to handle all the logic around the
devchannel? That doesn't exist in Flutter. I can usebeta, but would that really solve the issue? Or would these tests need to work on Dart before the Flutter team picks that SDK version for their next beta? For now, I switched the CI to Flutter, so let's see if it goes green this time.
Does the GitHub action support using the master channel? It's all about quick feedback.
The native_assets_builder/ tests that copy the test projects around now start failing because resolution requires a workspace. We should be able to remove the resolution: workspace entry in pkgs/native_assets_builder/test/helpers.dart copyTestProjects.
Does the GitHub action support using the master channel? It's all about quick feedback.
Done, switched to Flutter's main channel
The native_assets_builder/ tests that copy the test projects around now start failing because resolution requires a workspace. We should be able to remove the resolution: workspace entry in pkgs/native_assets_builder/test/helpers.dart copyTestProjects.
Done. I had to add dependency_overrides for native_assets_cli and native_toolchain_c in all packages, except for native_add_version_skew. All the infra issues in native_assets_builder tests are gone, but I'm just getting this error with VisualStudioResolver in the skew test. Could be an artifact of my system, let's see if it happens on CI.
Error output
Null check operator used on a null value
#0 VisualStudioResolver.resolve (package:native_toolchain_c/src/native_toolchain/msvc.dart:283:65)
<asynchronous suspension>
#1 RelativeToolResolver.resolve (package:native_toolchain_c/src/tool/tool_resolver.dart:259:32)
<asynchronous suspension>
#2 PathVersionResolver.resolve (package:native_toolchain_c/src/tool/tool_resolver.dart:151:27)
<asynchronous suspension>
#3 RelativeToolResolver.resolve (package:native_toolchain_c/src/tool/tool_resolver.dart:259:32)
<asynchronous suspension>
#4 CliVersionResolver.resolve (package:native_toolchain_c/src/tool/tool_resolver.dart:91:27)
<asynchronous suspension>
#5 CompilerResolver._tryLoadToolFromNativeToolchain (package:native_toolchain_c/src/cbuilder/compiler_resolver.dart:117:23)
<asynchronous suspension>
#6 CompilerResolver.resolveCompiler (package:native_toolchain_c/src/cbuilder/compiler_resolver.dart:45:18)
<asynchronous suspension>
#7 RunCBuilder.compiler (package:native_toolchain_c/src/cbuilder/run_cbuilder.dart:84:44)
<asynchronous suspension>
#8 RunCBuilder.run (package:native_toolchain_c/src/cbuilder/run_cbuilder.dart:115:50)
<asynchronous suspension>
#9 CBuilder.run (package:native_toolchain_c/src/cbuilder/cbuilder.dart:162:7)
<asynchronous suspension>
#10 main.<anonymous closure> (file:///c:/users/levi/appdata/local/temp/7d1c4bb/native_add_version_skew/hook/build.dart:19:5)
<asynchronous suspension>
#11 build (package:native_assets_cli/src/api/build.dart:101:3)
<asynchronous suspension>
#12 main (file:///c:/users/levi/appdata/local/temp/7d1c4bb/native_add_version_skew/hook/build.dart:10:3)
<asynchronous suspension>
stdout:
FINER: 2025-01-14 18:17:16.642465: No compiler set in BuildConfig.cCompiler.cc.
FINER: 2025-01-14 18:17:16.656951: Looking for Visual Studio Locator on PATH.
INFO: 2025-01-14 18:17:16.663228: Running `where vswhere.exe`.
SEVERE: 2025-01-14 18:17:16.892507: INFO: Could not find files for the given pattern(s).
FINE: 2025-01-14 18:17:16.909991: Did not find Visual Studio Locator on PATH.
FINER: 2025-01-14 18:17:16.912369: Looking for Visual Studio Locator in [C:/Program Files \(x86\)/Microsoft Visual Studio/Installer/vswhere.exe, C:/Program Files/Microsoft Visual Studio/Installer/vswhere.exe].
FINE: 2025-01-14 18:17:17.047220: Found [ToolInstance(Visual Studio Locator, null, file:///C:/Program%20Files%20(x86)/Microsoft%20Visual%20Studio/Installer/vswhere.exe)].
FINER: 2025-01-14 18:17:17.048224: Looking up version with --version for ToolInstance(Visual Studio Locator, null, file:///C:/Program%20Files%20(x86)/Microsoft%20Visual%20Studio/Installer/vswhere.exe).
INFO: 2025-01-14 18:17:17.050225: Running `C:\Program Files (x86)\Microsoft Visual Studio\Installer\vswhere.exe`.
FINE: 2025-01-14 18:17:17.148590: Visual Studio Locator version 3.1.7+f39851e70f [query version 3.7.2174.19405]
FINE: 2025-01-14 18:17:17.148590: Copyright (C) Microsoft Corporation. All rights reserved.
FINE: 2025-01-14 18:17:17.148590:
FINE: 2025-01-14 18:17:17.171589: Found version for ToolInstance(Visual Studio Locator, 3.1.7+f39851e70f, file:///C:/Program%20Files%20(x86)/Microsoft%20Visual%20Studio/Installer/vswhere.exe).
INFO: 2025-01-14 18:17:17.172591: Running `C:\Program Files (x86)\Microsoft Visual Studio\Installer\vswhere.exe`.
FINE: 2025-01-14 18:17:17.261727: Visual Studio Locator version 3.1.7+f39851e70f [query version 3.7.2174.19405]
FINE: 2025-01-14 18:17:17.261727: Copyright (C) Microsoft Corporation. All rights reserved.
FINE: 2025-01-14 18:17:17.261727:
Found why the health checks aren't finishing:
https://github.com/bmw-tech/dart_apitool/blob/e3b27c7f764ae34a00dd9f18bc2d68c146a5739a/lib/src/cli/commands/command_mixin.dart#L111-L116
https://github.com/bmw-tech/dart_apitool/blob/e3b27c7f764ae34a00dd9f18bc2d68c146a5739a/lib/src/cli/commands/command_mixin.dart#L294-L295
This is coming from package:dart_apitool. They're expecting each package to have its own .dart_tool directory, but that isn't true in the case of mono-repos. This is the case as of today, version 0.20.1. Thankfully, there's already a PR in the works at https://github.com/bmw-tech/dart_apitool/pull/202, with all checks green just 4 hours ago.
I merged main and fixed the other CI errors, so let's try it again. Also, my VS Code only takes up 500MB of RAM now, when analyzing the entire repo. I don't know how much it used to be, but I remember my computer getting really slow before!
Thanks a ton @Levi-Lesches! 🚀
This is coming from
package:dart_apitool. They're expecting each package to have its own.dart_tooldirectory, but that isn't true in the case of mono-repos. This is the case as of today, version0.20.1. Thankfully, there's already a PR in the works at bmw-tech/dart_apitool#202, with all checks green just 4 hours ago.
@mosuem Do we need a bump in the ecosystem repo to use the newest version of api_tool?
Also, my VS Code only takes up 500MB of RAM now, when analyzing the entire repo. I don't know how much it used to be, but I remember my computer getting really slow before!
🔥 😄
(They just released version 0.20.2 with the fix, and firehose picked it up automatically)
pubspec bump: I don't believe so, users on an older SDK will not have pub resolve to newer versions of the packages.
changelog: yes, bumping minimum SDK constraint should be in the changelog. (Also, soon I'd like to bump things to 3.7 when it's out, because I want the new formatter.)
Kinda confused here, since some of these packages are not -wip at the moment, so not bumping would mean modifying the changelog for versions that are already out there. So I just bumped the patch version (since no features were added) and added Dart 3.6 to the changelog under that new version.
Yet another CI issue: Testing native_assets_ci/example/build/native_dynamic_linking.dart fails:
PathNotFoundException: Cannot open file, path = 'D:\a\native\native\pkgs\native_assets_cli\example\build\native_add_app\.dart_tool\package_config.json' (OS Error: The system cannot find the file specified.
That's because the native_assets_builder package itself doesn't understand workspaces:
static Future<PackageLayout> fromRootPackageRoot(
FileSystem fileSystem,
Uri rootPackageRoot,
) async {
rootPackageRoot = rootPackageRoot.normalizePath();
final packageConfigUri =
rootPackageRoot.resolve('.dart_tool/package_config.json'); // <--
assert(await fileSystem.file(packageConfigUri).exists());
final packageConfig = await loadPackageConfigUri(packageConfigUri);
return PackageLayout._(
fileSystem, rootPackageRoot, packageConfig, packageConfigUri);
}
Also, it seems that ffigen somehow depends on native_assets_builder, although I can't see how:
dart testinpackge:ffiworks finedart testinpackage:ffigenfails because ofcyclic_package_1andcyclic_package_2
Package publishing
| Package | Version | Status | Publish tag (post-merge) |
|---|---|---|---|
| package:ffi | 2.1.4 | ready to publish | ffi-v2.1.4 |
| package:jni | 0.13.1 | ready to publish | jni-v0.13.1 |
| package:ffigen | 17.0.0-wip | WIP (no publish necessary) | |
| package:jnigen | 0.13.2-wip | WIP (no publish necessary) | |
| package:objective_c | 5.0.0-wip | WIP (no publish necessary) | |
| package:swift2objc | 0.0.1-wip | WIP (no publish necessary) | |
| package:swiftgen | 0.0.1-wip | WIP (no publish necessary) |
Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation.
dart testinpackage:ffigenfails because ofcyclic_package_1andcyclic_package_2
I've dove into this. It's because we don't properly pass the runPackageName everywhere. In package:dartdev we do, so dart run in package:ffi we only consider transitive dependencies of package:ffi. If we do flutter run in package:ffi we have the package:flutter_tools implementation, which does not pass the runPackageName. So then it starts trying to build native assets for all packages in the package graph, instead of only the packages which are transitive dependencies of package:ffi.
The same issue exists for the check if the native assets experiment should be enabled or not. It currently also looks at all packages in the package graph.
Before pub workspaces, if we didn't pass runPackageName it was always the root package. And without pub workspaces, that is identical to the package resolution graph. But now with pub workspaces, this is no longer true.
I'm going to fix this behavior in package:native_assets_builder and then roll it into dartdev and flutter tools. When the fix is in flutter master branch, we can disable the CI for flutter stable, and things should become more green.
Sounds good. And I found this in the firehose checks:
Running `/opt/hostedtoolcache/dart/3.7.0-309.0.dev/x64/bin/dart pub global activate -sgit https://github.com/bmw-tech/dart_apitool.git --git-ref 123049d3fa3c1459a5129b2b61d852a388a8511e` in /home/runner/work/native/native/current_repo
So there probably needs to be an update there to use the latest dart_apitool, since it's not using semver directly. Seems this PR is gonna have to wait for this and #1905 then. Feel free to ping me when those are resolved and glad I can help in the meantime!
Re: firehose, dart_apitool version is set here https://github.com/dart-lang/ecosystem/blob/94b4cea02f2948432a1c9218c573cc03d5ef144c/pkgs/firehose/lib/src/health/health.dart#L20C22-L20C62. I will file a PR to update it!
Filed https://github.com/dart-lang/ecosystem/pull/338
Edit: merged.
Package:jni used an older version of ffigen as a dev dependency because it relied on its internals. dart run tool/generate_ffi_bindings.dart in jni now doesn't work.
@HosseinYousefi I fixed as much as I could, see the diff here. The issue now is that I get
$ dart tool\generate_ffi_bindings.dart
INFO: Generating C wrappers
Unknown type ffi.UnsignedInt for return type
And I described some more details in https://github.com/dart-lang/native/pull/1884#issuecomment-2586047176
@HosseinYousefi I fixed as much as I could, see the diff here. The issue now is that I get
$ dart tool\generate_ffi_bindings.dart INFO: Generating C wrappers Unknown type ffi.UnsignedInt for return typeAnd I described some more details in https://github.com/dart-lang/native/pull/1884#issuecomment-2586047176
I will try to fix this part of your PR on Monday. Thanks!
Added a commit that fixes package:jni issues.
Fixed merge conflicts with the new native_assets changes -- if that's working, can we run CI again?
@Levi-Lesches The Dart version containing the fixes is https://github.com/dart-lang/sdk/releases/tag/3.8.0-28.0.dev. The next dart dev release (Tuesday next week), should contain the fixes. The Flutter master channel contains the fixes for flutter but the dart in Flutter master is still 3.8.0-24.0.dev.
Once Dart in Flutter rolls to 28.0.dev you should be able to rebase this PR (assuming all the GitHub actions will be using flutter master, and not Dart dev).
(Sorry for the merge mess, I had to release a new version of every package, so all pubspecs now have merge conflicts. 😄 )
Sounds good!
Sorry for the merge mess
At first I was confused how it got this bad, but then I realized I've never seen a workflow where in order to merge a PR, you must first merge parts of it, flag it for release, wait for the release to come out, then let CI run and hit merge 😂.
I'll try to remember about this in a week, but if I don't, feel free to ping me!
At first I was confused how it got this bad, but then I realized I've never seen a workflow where in order to merge a PR, you must first merge parts of it, flag it for release, wait for the release to come out, then let CI run and hit merge 😂.
Yeah, we're building Dart with Dart. (Where Dart and Dart are different versions of Dart.) 🤓
I'll try to remember about this in a week, but if I don't, feel free to ping me!
Friendly ping @Levi-Lesches!
It would be good split up this PR in separate PRs in which the last one introduces the pub workspace. This would (1) enable us to review separate aspects separately, and (2) land those separate aspects separately (avoiding any future merge issues with these changes 😄 ).
- A PR that bumps deps and addresses lints for
package:ffigen+package:objective_cto be reviewed by @liamappelbe - A PR that bumps deps and addresses lints for
package:swift2objcto be reviewed by @liamappelbe - A PR that bumps deps and addresses lints for
package:jni+package:jnigento be reviewed by @HosseinYousefi - A PR that bumps deps and addresses lints for
package:ffito be reviewed by me. - A PR that bumps deps and addresses lints for
package:native_to be reviewed by me. - A PR that does some of the github workflow cleanups, to be reviewed by me.
Then the actual flipping to a pub workspace (with the remaining github workflow) changes will possibly need some more iteration:
- FFIgen, package:ffi, JNIgen, JNI all need to run against Dart/Flutter stable
package:native_assets_climight want a dart dev release as a lower bound (https://github.com/dart-lang/native/issues/93#issuecomment-2624818266) which would force the whole workspace to have the dev release for that package I believe. --> I need to think a bit about what the right solution is there.
Landing all the prerequisite changes first, will make it easier for me to experiment with what's the right solution. (I would really like one workspace, so that we have one set of lints, one set of dependencies etc!)