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.

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

Move core ObjC classes into their own library #430

Closed
liamappelbe opened this issue Sep 3, 2023 · 22 comments · Fixed by #1088
Closed

Move core ObjC classes into their own library #430

liamappelbe opened this issue Sep 3, 2023 · 22 comments · Fixed by #1088
Assignees
Labels
lang-objective_c Related to Objective C support package:ffigen package:objective_c stable objc blocker Need for ObjC to be considered stable

Comments

@liamappelbe
Copy link
Contributor

For example NSObject and NSString. This makes sense from a modularisation perspective, and also solves the problem where 2 different code genned ObjC libraries are incompatible.

@liamappelbe liamappelbe added lang-objective_c Related to Objective C support stable objc blocker Need for ObjC to be considered stable labels Sep 3, 2023
@liamappelbe liamappelbe self-assigned this Sep 3, 2023
@dcharkes
Copy link
Collaborator

dcharkes commented Sep 4, 2023

We'll likely need to convert this repo to a monorepo (similar to JNIgen), that's a better approach than a new repo.

JNIgen also has a helper library package:jni with common types.

We should try to do that conversion at a point where not a ton of PRs are outstanding.

@vaind
Copy link
Contributor

vaind commented Sep 12, 2023

This would also solve the pain point I've had when creating this issue: #432 - although the issues are not the same, anything to get rid of those MiB of generated code would be nice.

@vaind
Copy link
Contributor

vaind commented Oct 31, 2023

We should try to do that conversion at a point where not a ton of PRs are outstanding.

Now seems to be the right time then :D

@dcharkes
Copy link
Collaborator

cc @devoncarew any concerns about converting this repo to a monorepo?

The helper package with code that the FFIgen generated could would import should be called:

  • package:objective_c (most obvious name, but does not indicate it belongs to FFIgen generated code)
  • package:objc (because it's shorter than the first)
  • package:ffigen_objc (so it's clear it belongs to FFIgen)
  • package:ffigen_objective_c (so it's clear it belongs to FFIgen)

cc @mit-mit for package name inspiration.

@devoncarew
Copy link
Member

cc @devoncarew any concerns about converting this repo to a monorepo?

None, but consider also moving this into dart-lang/native.

And, +1 to the package names qualifed w/ ffi prefixes (package:ffigen_objc, package:ffigen_objective_c, ...).

@dcharkes
Copy link
Collaborator

My reservation with moving FFIgen (and then probably also package:ffi, JNIgen and package:jni) all to dart-lang/native is that all issues will be in one big issue tracker. cc @HosseinYousefi @mannprerak2 @liamappelbe @brianquinlan as most impacted workflow-wise.

@HosseinYousefi
Copy link
Member

I like the idea of having all the native stuff in dart-lang/native.

all issues will be in one big issue tracker

This can be solved using a tag for each package.

@devoncarew
Copy link
Member

My reservation with moving FFIgen (and then probably also package:ffi, JNIgen and package:jni) all to dart-lang/native is that all issues will be in one big issue tracker.

That's a drawback for sure, but I think the cost of doing business; we do get real economies of scale from monorepos.

Here's an example of issues for a mono-repo; we just make sure that each package has an issue template, so all filed issues up up w/ a label to help categorize them: https://github.com/dart-lang/tools/issues.

@liamappelbe
Copy link
Contributor Author

I'm also in favour of a dart-lang/native monorepo. We need to start deduping logic between ffigen and jnigen.

@dcharkes
Copy link
Collaborator

dcharkes commented Nov 1, 2023

Okay!

The process of merging a (non-mono) repo into dart-lang/native can be found here: dart-lang/tools#100. @liamappelbe could you follow these steps for the FFIgen repo and make a PR in the same style?

@liamappelbe
Copy link
Contributor Author

I'm starting work on this now. Gonna call it package:ffigen_objc, unless anyone objects.

@dcharkes
Copy link
Collaborator

I'm starting work on this now. Gonna call it package:ffigen_objc, unless anyone objects.

How about:

  • package:ffi_objc or package:ffi_objectivec or package:ffi_objective_c? It's a helper package that doesn't do any generation but is targetted by code gen. It's like the helpers in package:ffi and package:jni but for the context Objective-C.
  • (package:objective_c. I like this slightly less, because it does not refer to FFI in any way. E.g. it could be a package that generates Objective-C instead of that interops with Objective-C.)

I don't see many guidelines for package names (besides https://dart.dev/tools/linter-rules/package_names).

cc @mit-mit @jonasfj @lrhn for package name

@lrhn
Copy link
Member

lrhn commented Mar 19, 2024

How will the package be imported? Will the user have to import it, or will ffi_gen inject an import?
(In the latter case, the user still needs a non-dev dependency on the package.)

Could it be a separate libraries of package:ffi, as package:ffi/objective_c.dart. If nothing else, the user presumably has a dependency on package:ffi already. (The title says "its own library", not "its own package", so ....?)

@dcharkes
Copy link
Collaborator

dcharkes commented Mar 19, 2024

How will the package be imported? Will the user have to import it, or will ffi_gen inject an import? (In the latter case, the user still needs a non-dev dependency on the package.)

The latter. (It is similar to package:jni, the generated code will import it.)

Could it be a separate libraries of package:ffi, as package:ffi/objective_c.dart. If nothing else, the user presumably has a dependency on package:ffi already. (The title says "its own library", not "its own package", so ....?)

It could. However, I am slightly worried about breaking changes. Objective C support is still experimental. package:ffi is used all over the place, and users are already using dependency overrides because they have dependencies using v1.x.x and v2.x.x. I'd like to avoid having to bump package:ffi's major version because we update package:ffi/objective_c.dart in a breaking way. @liamappelbe Do you expect us to do breaking changes to package:ffi_objective_c while iterating on it?

Edit: Especially a package which is used in conjunction with a code generator, it is quite common to change the pattern of the generated code. package:jni also had multiple breaking changes already.

@lrhn
Copy link
Member

lrhn commented Mar 19, 2024

The latter. (It is similar to package:jni, the generated code will import it.)

Which means that the code using ffi_gen needs to have a non-dep dependency on the package.

In that case, I think it's better to put the files inside package:ffi and only have one package dependency.

The breaking change for experimental code is an issue, but that should end eventually. Could you just mark the library as @experimental, and break it with abandon?

If we expect breaking changes post experiment stage, then linking things that evolve independently is an issue, since it can introduce too many major-version increments. Then separate packages is probably the right thing.

In that case, I'd go with package:ffi_objective_c if the package cannot be used directly, only through ffi_gen.
If it can be used directly as well, then package:objective_c is fine.
(Can package:jni be used by itself, or should it be called package:ffi_jni?)

@dcharkes
Copy link
Collaborator

(Can package:jni be used by itself, or should it be called package:ffi_jni?)

JNI is already "Java native interface", so it is inherently FFI related. If it was package:java we should have named it package:ffi_java.

The breaking change for experimental code is an issue, but that should end eventually. Could you just mark the library as @experimental, and break it with abandon?

I believe tools such as https://pub.dev/packages/dart_apitool have poor support for marking some libraries in a package as experimental and ignoring breaking changes. So this would constantly turn our CI red.

Maybe we should go for package:ffi_objective_c for now, and when Objective-C interop stabilizes and gets out of experimental we can evaluate whether we'd want to merge it into package:ffi as package:ffi/objective_c.dart.

@HosseinYousefi
Copy link
Member

HosseinYousefi commented Mar 19, 2024

Gonna call it package:ffigen_objc, unless anyone objects.

Having gen in the name might suggest that it is a code generator.

If we expect breaking changes post experiment stage.

I would personally expect that based on my experience.

(Can package:jni be used by itself, or should it be called package:ffi_jni?)

It can be used by itself. We have a package:jni/internal_helpers_for_jnigen.dart as well (I know, long name, but I kept it the way it was!) that exposes certain functionalities that only package:jnigen needs.

The thing about package:jni is that we might decide to decouple it from some future package:java_core and package:kotlin_something. Where package:jni only has the very basic classes like JReference and JObject and all the other classes exist in a different package. But again it's a good default for jnigen and ffigen to use the classes inside package:java_core and package:objc_foundation(?) by default.

JNI is already "Java native interface", so it is inherently FFI related.

Am I the only one who doesn't really mind not directly implying the fact that these packages are ffi related in the name? (Afterall, how else are we working with Objective-C or Java, or any other language from Dart standalone!)

@liamappelbe
Copy link
Contributor Author

@liamappelbe Do you expect us to do breaking changes to package:ffi_objective_c while iterating on it?

Definitely. My experience has been that generated code changes more often and more drastically than normal code.

Based on everyone's feedback, I'm leaning towards package:ffi_objc.

@dcharkes
Copy link
Collaborator

dcharkes commented Mar 21, 2024

@liamappelbe Do you expect us to do breaking changes to package:ffi_objective_c while iterating on it?

Definitely. My experience has been that generated code changes more often and more drastically than normal code.

👍

Based on everyone's feedback, I'm leaning towards package:ffi_objc.

I'd prefer package:ffi_objective_c, because I believe it would be better to avoid abbreviations. (Although, we do have ffigen where generator is abbreviated to gen. So maybe this isn't the strongest argument. 😆) Happy with either of the two.

@HosseinYousefi
Copy link
Member

@liamappelbe Do you expect us to do breaking changes to package:ffi_objective_c while iterating on it?

Definitely. My experience has been that generated code changes more often and more drastically than normal code.

👍

Based on everyone's feedback, I'm leaning towards package:ffi_objc.

I'd prefer package:ffi_objective_c, because I believe it would be better to avoid abbreviations. (Although, we do have ffigen where generator is abbreviated to gen. So maybe this isn't the strongest argument. 😆) Happy with either of the two.

I like package:objective_c.

As @lrhn said:

If it can be used directly as well, then package:objective_c is fine.

And I think it can be used directly.

@liamappelbe
Copy link
Contributor Author

And I think it can be used directly.

Fair enough. @dcharkes any objections to package:objective_c? I would prefer package:objc, but I don't think I'm going to win that argument 😛

@dcharkes
Copy link
Collaborator

And I think it can be used directly.

Fair enough. @dcharkes any objections to package:objective_c? I would prefer package:objc, but I don't think I'm going to win that argument 😛

sgtm!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lang-objective_c Related to Objective C support package:ffigen package:objective_c stable objc blocker Need for ObjC to be considered stable
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

6 participants