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.

Sign up for GitHub

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

Fix flutter health wf #1787

Merged
merged 18 commits into from
Jan 2, 2025
Merged

Fix flutter health wf #1787

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
091aeb8
Fix flutter health wf
mosuem Dec 6, 2024
80c3823
change paths
mosuem Dec 6, 2024
8491b7a
use branch
mosuem Dec 6, 2024
7766d47
Ignore swiftgen
mosuem Dec 6, 2024
1b47414
Ignore ffigen
mosuem Dec 6, 2024
a7b3c61
disable jnigen
mosuem Dec 6, 2024
34fe2ea
Dont ignore anymore
mosuem Dec 9, 2024
722ba0c
ignore swiftgen
mosuem Dec 9, 2024
86ac59b
Fix paths
mosuem Dec 9, 2024
285b15d
use package from path
mosuem Dec 9, 2024
ee6b78e
Don't check jnigen
mosuem Dec 9, 2024
eaf9043
Add changelog
mosuem Dec 9, 2024
ff3c3e4
Merge branch 'main' into fixFlutterHealth
mosuem Dec 9, 2024
e6de73c
Dont check swift2objc
mosuem Dec 9, 2024
f51cdf5
Use main channel
mosuem Dec 9, 2024
7c6e300
Rev versions according to breaking change policy
mosuem Dec 12, 2024
c50b12e
Rev version even more
mosuem Dec 12, 2024
58950af
Run on all branches
mosuem Jan 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 9 additions & 8 deletions .github/workflows/health.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,23 @@
name: Health
on:
pull_request:
branches: [ main ]
# Remove paths after https://github.com/bmw-tech/dart_apitool/issues/177 is addressed.
paths:
- "pkgs/ffi/**"
- "pkgs/native_assets_builder/**"
- "pkgs/native_assets_cli/**"
- "pkgs/native_toolchain_c/**"
- ".github/workflows/health.yaml"
- "pkgs/**"
types: [opened, synchronize, reopened, labeled, unlabeled]
jobs:
health:
uses: dart-lang/ecosystem/.github/workflows/health.yaml@main
with:
coverage_web: false
# TODO(https://github.com/dart-lang/native/issues/1242): Add coverage back.
checks: "version,changelog,license,do-not-submit,breaking,leaking"
use-flutter: true
sdk: master
checks: "changelog,license,do-not-submit,breaking,leaking"
flutter_packages: "pkgs/ffigen,pkgs/jni,pkgs/jnigen,pkgs/objective_c"
ignore_license: "**.g.dart"
ignore_coverage: "**.mock.dart,**.g.dart"
ignore_packages: "pkgs/swiftgen,pkgs/jnigen,pkgs/swift2objc"
sdk: dev
channel: master
mosuem marked this conversation as resolved.
Show resolved Hide resolved
permissions:
pull-requests: write
21 changes: 0 additions & 21 deletions .github/workflows/health_flutter.yaml
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion pkgs/ffigen/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
## 16.0.1-wip
## 17.0.0-wip

Copy link
Contributor

Choose a reason for hiding this comment

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

What's this change for? Can we make it 16.1 instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's because ffigen has breaking changes, see the PR health comment. Happy to disable the breaking change check for ffigen in general, address the upgrade in a later PR, or just ignore this breakage.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the issue is that the breaking change checker is checking for changes on internal classes. This is the only breaking change in the report:

  "breakingChanges": {
    "label": "BREAKING CHANGES",
    "children": [
      {
        "label": "Class ImportedType",
        "children": [
          {
            "label": "Constructor ImportedType",
            "children": [
              {
                "changeDescription": "Kind of parameter \"defaultValue\" changed. not named -> named",
                "changeCode": "CE07",
                "isBreaking": true,
                "type": "major"
              }
            ]
          }
        ]
      }
    ]
  },

ImportedType is not part of the public API (or at least, it shouldn't be public). The fix here is to figure out why that class (and others mentioned in the non-breaking changes) are considered to be part of the public API. It's probably related to the issues in the API leaks check. But if you want to just ignore ffigen for now, that's fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right - the leaks could have something to do with that. Those clean ups should be done in follow ups.

Copy link
Contributor

Choose a reason for hiding this comment

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

So can we leave the version as 16.1 and just disable the breaking change detector for ffigen for now?

- Ensure that required symbols are available to FFI even when the final binary
is linked with `-dead_strip`.
Expand Down
4 changes: 4 additions & 0 deletions pkgs/ffigen/example/shared_bindings/generate.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
// Copyright (c) 2024, 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.

import 'dart:io';

import 'package:cli_util/cli_util.dart';
Expand Down
2 changes: 1 addition & 1 deletion pkgs/ffigen/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
# BSD-style license that can be found in the LICENSE file.

name: ffigen
version: 16.0.1-wip
version: 17.0.0-wip
description: >
Generator for FFI bindings, using LibClang to parse C, Objective-C, and Swift
files.
Expand Down
1 change: 1 addition & 0 deletions pkgs/jnigen/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
- Fixed a potential name collision when generating in multi-file mode.
- Added the ability to add user-defined visitors to config. Currently only
capable of excluding classes, methods, and fields.
- Add dependency override for `package:jni` instead of the path dependency.

## 0.12.2

Expand Down
7 changes: 5 additions & 2 deletions pkgs/jnigen/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,10 @@ dependencies:
dev_dependencies:
build_runner: ^2.4.12
dart_flutter_team_lints: ^3.2.0
jni:
path: ../jni
jni: ^0.12.2
json_serializable: ^6.8.0
test: ^1.25.8

dependency_overrides:
jni:
path: ../jni
2 changes: 1 addition & 1 deletion pkgs/objective_c/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
## 4.0.1-wip
## 4.1.0-wip

- Reduces the chances of duplicate symbols by adding a `DOBJC_` prefix.
- Ensure that required symbols are available to FFI even when the final binary
Expand Down
2 changes: 1 addition & 1 deletion pkgs/objective_c/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

name: objective_c
description: 'A library to access Objective C from Flutter that acts as a support library for package:ffigen.'
version: 4.0.1-wip
version: 4.1.0-wip
repository: https://github.com/dart-lang/native/tree/main/pkgs/objective_c
issue_tracker: https://github.com/dart-lang/native/issues?q=is%3Aissue+is%3Aopen+label%3Apackage%3Aobjective_c

Expand Down
Loading