native icon indicating copy to clipboard operation
native copied to clipboard

[infra] Use Pub Workspaces

Open Levi-Lesches opened this issue 11 months ago • 32 comments

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 with dart pub get
  • [x] Verify that native_assets_cli/example/* still works with dart pub get
  • [x] Verify that dart analyze passes at the workspace level`
  • [x] Bump CI to use Dart 3.6 for all packages (stable/beta, migrated dev -> stable)
  • [x] Consolidate analysis_options.yaml into workspace
  • [ ] Verify tests still run (waiting for CI)
  • [x] Update changelogs and pubspec versions (Is this needed?)

Leaving these packages unmigrated:

  • jnigen/android_test_runner
  • native_assets_builder/test_data/native_add_version_skew

Blocked by:

  • #1905
  • package:firehose needs to use package:dart_apitool: ^0.20.2

  • [x] I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

Levi-Lesches avatar Jan 12 '25 23:01 Levi-Lesches

@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

Levi-Lesches avatar Jan 12 '25 23:01 Levi-Lesches

Possibly silly question, but does this require a pubspec bump and changelog addition?

Levi-Lesches avatar Jan 13 '25 01:01 Levi-Lesches

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.

Levi-Lesches avatar Jan 13 '25 02:01 Levi-Lesches

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?

Levi-Lesches avatar Jan 13 '25 04:01 Levi-Lesches

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)

Levi-Lesches avatar Jan 13 '25 04:01 Levi-Lesches

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

github-actions[bot] avatar Jan 13 '25 07:01 github-actions[bot]

Coverage Status

coverage: 83.95% (-4.6%) from 88.557% when pulling bf5d11cc23b7879708aa16e4222531ccb7929b4e on Levi-Lesches:workspaces into 3ed5d3d07246cd792693584c2c42216739a634b7 on dart-lang:main.

coveralls avatar Jan 13 '25 07:01 coveralls

🙏 !

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, 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, 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.

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 get command needs to be flutter instead of dart?
  • pub get commands 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.

dcharkes avatar Jan 13 '25 08:01 dcharkes

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.

Levi-Lesches avatar Jan 13 '25 19:01 Levi-Lesches

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?

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

liamappelbe avatar Jan 14 '25 03:01 liamappelbe

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.

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.

dcharkes avatar Jan 14 '25 07:01 dcharkes

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!

Levi-Lesches avatar Jan 14 '25 23:01 Levi-Lesches

Thanks a ton @Levi-Lesches! 🚀

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 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!

🔥 😄

dcharkes avatar Jan 15 '25 19:01 dcharkes

(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.

Levi-Lesches avatar Jan 15 '25 20:01 Levi-Lesches

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);
  }

Levi-Lesches avatar Jan 15 '25 21:01 Levi-Lesches

Also, it seems that ffigen somehow depends on native_assets_builder, although I can't see how:

  • dart test in packge:ffi works fine
  • dart test in package:ffigen fails because of cyclic_package_1 and cyclic_package_2

Levi-Lesches avatar Jan 15 '25 21:01 Levi-Lesches

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.

github-actions[bot] avatar Jan 15 '25 22:01 github-actions[bot]

  • dart test in package:ffigen fails because of cyclic_package_1 and cyclic_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.

dcharkes avatar Jan 16 '25 10:01 dcharkes

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!

Levi-Lesches avatar Jan 16 '25 22:01 Levi-Lesches

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!

mosuem avatar Jan 17 '25 08:01 mosuem

Filed https://github.com/dart-lang/ecosystem/pull/338

Edit: merged.

mosuem avatar Jan 17 '25 08:01 mosuem

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 avatar Jan 17 '25 16:01 HosseinYousefi

@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

Levi-Lesches avatar Jan 17 '25 19:01 Levi-Lesches

@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

I will try to fix this part of your PR on Monday. Thanks!

HosseinYousefi avatar Jan 17 '25 19:01 HosseinYousefi

Added a commit that fixes package:jni issues.

HosseinYousefi avatar Jan 21 '25 18:01 HosseinYousefi

Fixed merge conflicts with the new native_assets changes -- if that's working, can we run CI again?

Levi-Lesches avatar Jan 21 '25 19:01 Levi-Lesches

@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. 😄 )

dcharkes avatar Jan 24 '25 08:01 dcharkes

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!

Levi-Lesches avatar Jan 24 '25 09:01 Levi-Lesches

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.) 🤓

dcharkes avatar Jan 24 '25 10:01 dcharkes

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_c to be reviewed by @liamappelbe
  • A PR that bumps deps and addresses lints for package:swift2objc to be reviewed by @liamappelbe
  • A PR that bumps deps and addresses lints for package:jni + package:jnigen to be reviewed by @HosseinYousefi
  • A PR that bumps deps and addresses lints for package:ffi to 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_cli might 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!)

dcharkes avatar Jan 30 '25 16:01 dcharkes