-
Notifications
You must be signed in to change notification settings - Fork 42
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 converter from Dart Stream
to NSInputStream
#1555
Conversation
PR HealthChangelog Entry ✔️
Changes to files need to be accounted for in their respective changelogs.
Coverage
|
File | Coverage |
---|---|
pkgs/ffigen/lib/src/code_generator/objc_built_in_functions.dart | 💔 Not covered |
pkgs/ffigen/lib/src/code_generator/objc_protocol.dart | 💔 Not covered |
pkgs/objective_c/lib/objective_c.dart | 💔 Not covered |
pkgs/objective_c/lib/src/ns_input_stream.dart | 💔 Not covered |
pkgs/objective_c/lib/src/objective_c_bindings_generated.dart | 💔 Not covered |
This check for test coverage is informational (issues shown here will not fail the PR).
This check can be disabled by tagging the PR with skip-coverage-check
.
License Headers ⚠️
// 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.
Files |
---|
pkgs/objective_c/lib/src/ns_input_stream.dart |
All source files should start with a license header.
Unrelated files missing license headers
Files |
---|
pkgs/ffigen/example/libclang-example/generated_bindings.dart |
pkgs/ffigen/example/shared_bindings/generate.dart |
pkgs/ffigen/example/shared_bindings/lib/generated/a_gen.dart |
pkgs/ffigen/example/shared_bindings/lib/generated/a_shared_b_gen.dart |
pkgs/ffigen/example/shared_bindings/lib/generated/base_gen.dart |
pkgs/ffigen/example/simple/generated_bindings.dart |
pkgs/ffigen/lib/src/header_parser/clang_bindings/clang_bindings.dart |
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_decl_decl_collision_bindings.dart |
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_decl_symbol_address_collision_bindings.dart |
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_decl_type_name_collision_bindings.dart |
pkgs/ffigen/test/collision_tests/expected_bindings/_expected_reserved_keyword_collision_bindings.dart |
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_comment_markup_bindings.dart |
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_dart_handle_bindings.dart |
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_enum_int_mimic_bindings.dart |
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_forward_decl_bindings.dart |
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_functions_bindings.dart |
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_imported_types_bindings.dart |
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_native_func_typedef_bindings.dart |
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_opaque_dependencies_bindings.dart |
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_packed_structs_bindings.dart |
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_regress_384_bindings.dart |
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_struct_fptr_fields_bindings.dart |
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_typedef_bindings.dart |
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_unions_bindings.dart |
pkgs/ffigen/test/header_parser_tests/expected_bindings/_expected_varargs_bindings.dart |
pkgs/ffigen/test/large_integration_tests/_expected_cjson_bindings.dart |
pkgs/ffigen/test/large_integration_tests/_expected_libclang_bindings.dart |
pkgs/ffigen/test/large_integration_tests/_expected_sqlite_bindings.dart |
pkgs/ffigen/test/native_test/_expected_native_test_bindings.dart |
pkgs/jni/lib/src/third_party/generated_bindings.dart |
pkgs/jni/lib/src/third_party/global_env_extensions.dart |
pkgs/jni/lib/src/third_party/jni_bindings_generated.dart |
pkgs/jnigen/android_test_runner/lib/main.dart |
pkgs/jnigen/example/in_app_java/lib/android_utils.dart |
pkgs/jnigen/example/kotlin_plugin/example/lib/main.dart |
pkgs/jnigen/example/kotlin_plugin/lib/kotlin_bindings.dart |
pkgs/jnigen/example/kotlin_plugin/lib/kotlin_plugin.dart |
pkgs/jnigen/example/pdfbox_plugin/lib/pdfbox_plugin.dart |
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/pdmodel/PDDocument.dart |
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/pdmodel/PDDocumentInformation.dart |
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/pdmodel/_package.dart |
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/text/PDFTextStripper.dart |
pkgs/jnigen/example/pdfbox_plugin/lib/src/third_party/org/apache/pdfbox/text/_package.dart |
pkgs/jnigen/lib/src/bindings/descriptor.dart |
pkgs/jnigen/lib/src/elements/elements.g.dart |
pkgs/jnigen/test/jackson_core_test/third_party/bindings/com/fasterxml/jackson/core/_package.dart |
pkgs/jnigen/tool/command_runner.dart |
pkgs/native_assets_builder/test_data/native_dynamic_linking/bin/native_dynamic_linking.dart |
pkgs/swift2objc/lib/src/config.dart |
pkgs/swift2objc/lib/src/generate_wrapper.dart |
pkgs/swift2objc/lib/src/generator/_core/utils.dart |
pkgs/swift2objc/lib/src/generator/generator.dart |
pkgs/swift2objc/lib/src/generator/generators/class_generator.dart |
pkgs/swift2objc/lib/src/parser/parsers/declaration_parsers/parse_initializer_declaration.dart |
pkgs/swift2objc/lib/src/parser/parsers/declaration_parsers/parse_property_declaration.dart |
pkgs/swift2objc/lib/src/transformer/_core/unique_namer.dart |
pkgs/swift2objc/lib/src/transformer/_core/utils.dart |
pkgs/swift2objc/lib/src/transformer/transformers/transform_property.dart |
pkgs/swift2objc/test/unit/parse_initializer_param_test.dart |
This check can be disabled by tagging the PR with skip-license-check
.
Package publish validation ✔️
Package | Version | Status |
---|---|---|
package:ffi | 2.1.3 | already published at pub.dev |
package:ffigen | 15.0.0-wip | WIP (no publish necessary) |
package:jni | 0.12.0 | already published at pub.dev |
package:jnigen | 0.12.0 | already published at pub.dev |
package:native_assets_cli | 0.9.0-wip | WIP (no publish necessary) |
package:objective_c | 3.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.
NSStreamStatus _status; | ||
BOOL _done; | ||
NSError *_error; | ||
id<NSStreamDelegate> _delegate; // This is a weak reference. |
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.
Does this need a __weak
annotation? I thought ARC would treat this as a strong ref by default.
Also, if we're dealing with weak ref stuff, the test probably needs a ref counting test to make sure it's all working as expected. There aren't any ref counting tests in package:objective_c yet, so you'll have to copy across the utils from ffigen (objectRetainCount
and doGC
). Take a look at pkgs/ffigen/test/native_objc_test/arc_test.dart to see how they work. Ignore the stuff that uses counter
(that's a bit redundant since I added objectRetainCount
). Just check that objectRetainCount
> 0, drop your Dart references, do a GC, then check that objectRetainCount
== 0.
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.
Does this need a
__weak
annotation? I thought ARC would treat this as a strong ref by default.
By default, _delegate
is self
so I think that you need the reference to be weak to prevent the circular reference:
Also, if we're dealing with weak ref stuff, the test probably needs a ref counting test to make sure it's all working as expected. There aren't any ref counting tests in package:objective_c yet, so you'll have to copy across the utils from ffigen (
objectRetainCount
anddoGC
). Take a look at pkgs/ffigen/test/native_objc_test/arc_test.dart to see how they work. Ignore the stuff that usescounter
(that's a bit redundant since I addedobjectRetainCount
). Just check thatobjectRetainCount
> 0, drop your Dart references, do a GC, then check thatobjectRetainCount
== 0.
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.
By default,
_delegate
isself
so I think that you need the reference to be weak to prevent the circular reference:
Yeah, I mean shouldn't you add an explicit __weak
annotation? The way it's written now, I think ARC will be treating this field as strong.
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.
Got it. Fixed.
@@ -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: 2.0.0 | |||
version: 3.0.0-wip |
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.
Why does this need to be a major version bump?
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.
Doesn't the addition of any new classes affect the code that should be generated by package:ffigen
? For example, if we release package:objective_c
with this change, then existing versions of package:ffigen
may use it and users may face duplicate symbols: NSStream
defined in package:objective_c
and NSStream
defined by ffigen
ed code.
Or do I misunderstand how this works?
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.
Yeah, I guess we do need a major version bump. That's annoying. I've been treating this sort of thing as a minor version.
Also, why does it say that the coverage of |
I thought that objective-c objects were transmittable through send ports? Is the CI using an older flutter version, or something?
|
For test/interface_lists_test.dart, I was not intending to export Do you think that I should add a list of exceptions to the test? |
Yeah, this is a known issue. Coveralls doesn't always pick up all the subprojects. I haven't made any progress in debugging it. It's very annoying. |
Does the test pass locally? We have another test that sends ObjC objects through ports, and they pass on CI, so I don't think it's the flutter version. Sometimes capturing stuff in closures like this can cause problems, so you could try rewriting the test to make the |
@liamappelbe I'm guessing from your comments that you think that we should land this - I wasn't sure if this was general enough.
They do pass locally. I'll investigate further.
Any thoughts about this? |
Seems fine to me. What makes it not general enough?
Yeah, that makes sense. You can add it as an exception. |
/// the [Stream]. | ||
/// | ||
/// > [!IMPORTANT] | ||
/// > [NSInputStream.read_maxLength_] must called called from a different |
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.
nit: must called be called
@@ -4,4 +4,5 @@ | |||
|
|||
// Relative import to be able to reuse the ObjC sources. | |||
// See the comment in ../objective_c.podspec for more information. | |||
#include "../../src/input_stream_adapter.m" |
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.
You'll also need to add this to ios/Classes/objective_c.m
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.
Done.
})); | ||
} | ||
|
||
void main() { |
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.
Are you still working on the ref counting test? I want to make sure the __weak
annotation works as we expect. Just create a stream (one that has _delegate = self
), pipe some data through it, get a raw pointer to the stream and verify objectRetainCount != 0
, drop the reference to the stream, call doGC()
, then verify that the raw pointer now has a retain count of 0.
If blocks are involved, you might need to await a duration 0 future then do a second GC. The objectRetainCount
function depends on a native function defined in pkgs/ffigen/test/native_objc_test/util.h, so you'll also need to include that in the test dylib.
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.
Are you still working on the ref counting test? I want to make sure the
__weak
annotation works as we expect. Just create a stream (one that has_delegate = self
), pipe some data through it, get a raw pointer to the stream and verifyobjectRetainCount != 0
, drop the reference to the stream, calldoGC()
, then verify that the raw pointer now has a retain count of 0.If blocks are involved, you might need to await a duration 0 future then do a second GC. The
objectRetainCount
function depends on a native function defined in pkgs/ffigen/test/native_objc_test/util.h, so you'll also need to include that in the test dylib.
@liamappelbe I think that this PR is ready except for this. How do you think that we should reuse the code from pkgs/ffigen
? Should we move that functionality into a separate test-only package? Duplicate them in this package?
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.
I would just import them directly using relative paths (other tests already do this). If that won't work in this case, just duplicate them.
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.
@liamappelbe The objectRetainCount
implementation depends on native code that would need to be built. Could I ask you to do the infrastructure work and I can modify my test afterwards?
FWIW, when I run the tests, the number of DartInputStreamAdapter
deallocations match the number of allocations.
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.
LGTM. Just had a question about the test.
}); | ||
|
||
test('assign to non-self', () async { | ||
inputStream.delegate = [1, 2, 3].toNSData(); |
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.
I don't really understand how this delegate stuff works. Is this effectively swapping out the backing data stream of inputStream
for this NSData
? If so, the NSData
contains the same data that the inputStream
was constructed with, so the test could pass even if the delegate logic was broken. Can you change the NSData
's data?
Just for my own understanding, how can you use an NSData
as a NSStreamDelegate
? It doesn't seem to conform to that protocol.
Also, can you explain the control flow that you expect in this case? It's super hard to follow (mostly because I'm not very experienced with the delegate pattern). It seems that NSStreamDelegate
only has one method, and your implementation of that method at input_stream_adapter.m:162 only ever logs throws an error, or does nothing. What code actually invokes the methods on the delegate?
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.
I don't really understand how this delegate stuff works. Is this effectively swapping out the backing data stream of
inputStream
for thisNSData
? If so, theNSData
contains the same data that theinputStream
was constructed with, so the test could pass even if the delegate logic was broken. Can you change theNSData
's data?Just for my own understanding, how can you use an
NSData
as aNSStreamDelegate
? It doesn't seem to conform to that protocol.Also, can you explain the control flow that you expect in this case? It's super hard to follow (mostly because I'm not very experienced with the delegate pattern). It seems that
NSStreamDelegate
only has one method, and your implementation of that method at input_stream_adapter.m:162 only ever logs throws an error, or does nothing. What code actually invokes the methods on the delegate?
I changed the delegate method to just passthrough to the delegate (if it is not self
) and confirmed the behavior in tests. I also added NSStreamDelegate
to the list of protocols that exist in package:objective_c
.
Do you understand the global refcount failure?
Contribution guidelines:
dart format
.Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.