-
Notifications
You must be signed in to change notification settings - Fork 84
[objective_c] Migrate to native assets #2329
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
Conversation
PR Health
License Headers
|
| Files |
|---|
| pkgs/objective_c/example/command_line/lib/main.dart |
| pkgs/objective_c/lib/src/ns_input_stream.dart |
All source files should start with a license header.
This check can be disabled by tagging the PR with skip-license-check.
API leaks ⚠️
The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.
| Package | Leaked API symbol | Leaking sources |
|---|---|---|
| objective_c | _FinalizablePointer | internal.dart::_ObjCReference::new::_finalizable |
This check can be disabled by tagging the PR with skip-leaking-check.
Breaking changes ⚠️
| Package | Change | Current Version | New Version | Needed Version | Looking good? |
|---|---|---|---|---|---|
| objective_c | Breaking | 9.1.0 | 9.2.0-dev | 10.0.0 Got "9.2.0-dev" expected >= "10.0.0" (breaking changes) |
This check can be disabled by tagging the PR with skip-breaking-check.
Changelog Entry ✔️
| Package | Changed Files |
|---|
Changes to files need to be accounted for in their respective changelogs.
This check can be disabled by tagging the PR with skip-changelog-check.
@dcharkes I haven't been able to find a minimal repro for this, and it's a bit hard to debug as the build system is a bit of a black box to me. When I run ffigen tests that depend on package:objective_c, I get this error where it failed to load package:objective_c's native asset: The dylib is being built, and exists at the correct path ( |
This definitely means no native assets mapping is bundled with the Dart kernel/source file that is being run for the tests. My hunch would be that something triggers the build hook, and it writes the assets. However, that whatever compiles the test kernel file doesn't pick up on those assets. I had to write some code in package test to ensure the native assets yaml file is picked up when compiling the kernel file https://github.com/dart-lang/test/pulls?q=is%3Apr+author%3Adcharkes+is%3Aclosed. I wouldn't be surprised if you'd need something similar for package coverage.
It's not supposed to search any directories. The |
|
@dcharkes I made some changes to hook/build.dart to fix the flutter example build. Do these changes look reasonable? |
Yes, they look reasonable! |
Notes
@Nativeannotations to load all the ffi symbols. Previously this worked because I loaded the dylib into the executable first, then relied on the@Nativefallback behavior to load the symbols from the exe. Now I'm explicitly setting the asset name to the built dylib, which appears to disable that fallback behavior. But all the core ObjC runtime symbols (egobjc_msgSend) are linked into the exe, not the dylib. So I had to split the C bindings into a set that loads from the dylib, and a set that loads from the exe. There are now 3 sets of ffigen created bindings: the objective C bindings (loads from the dylib), the bindings for my C code (loads from the dylib), and the bindings for the built in ObjC runtime's C code (loads from the exe).Pointer<ObjCObject>, so I had to manually wrap them inNSStrings in globals.dart. I tried splitting the ObjC bindings like I split the C bindings, but this caused all sorts of other problems.test_with_coveragetools#2237, I've switched the ffigen coverage collection fromcoverage:test_with_coveragetodart test --coverage_path. This slightly reduces coverage because we don't have as much control over the filtering, so I can't tell it to retain the package:objective_c coverage info gathered during the ffigen tests.