sdk icon indicating copy to clipboard operation
sdk copied to clipboard

Expose `architecture` from `Abi` class

Open miquelbeltran opened this issue 1 year ago • 8 comments
trafficstars

Hello Dart team!

Currently, the easiest way to obtain the compiled architecture of an app is to call to Abi.current().toString() and then parse the String, e.g.

final abi = Abi.current().toString();
final architecture = abi.split('_').last;

Could it be possible to expose the Architecture property directly?

Related ticket: https://github.com/dart-lang/sdk/issues/40231

miquelbeltran avatar Aug 20 '24 07:08 miquelbeltran

Summary: The Abi class currently requires parsing a string to obtain the compiled architecture. Exposing the architecture property directly would simplify this process and improve code readability.

dart-github-bot avatar Aug 20 '24 07:08 dart-github-bot

As Josh Bloch said in Effective Java, don't provide information in toString that you don't provide otherwise. People will parse toString, and then its format becomes part of your API.

lrhn avatar Aug 20 '24 10:08 lrhn

Even the Dart team does it ;)

https://github.com/dart-lang/native/blob/4a18bf2964b69075d9ec3ec1295d4985494741bb/pkgs/native_assets_cli/test/api/target_test.dart#L18

miquelbeltran avatar Aug 20 '24 11:08 miquelbeltran

There's probably a reason Josh Bloch said that - everybody does it, until they learn why not to, often the hard way. And sometimes the toString should just stop leaking the information. The toString of an Abi can be seen as an opaque ID, or only intended to be shown for debugging. Or not, only the author knows.

(If the test is to trust, just use Architecture.current.toString(), it should be the same string as the second part of Abi.current.toString(). Not that it's more efficient, it just extracts the same string from Platform.version. And you need to depend on the native_assets_cli package.)

The Abi class could choose to expose osString and architectureString getters that return the two parts, unless the class expects to support other formats in the future, which may not be a combination of OS and CPU architecture.

lrhn avatar Aug 20 '24 11:08 lrhn

Yes, not disagreeing with you. The native_assets_cli package reverses the Abi class to extract the architecture, which looks better than parsing the toString:

https://github.com/dcharkes/native_assets_cli/blob/d9ee385ae127232f95b576968a5e135b14223d11/lib/src/model/target.dart#L44-L66

Alternatively, we could add an extension over Abi that does something similar to what the Architecture class is doing.

miquelbeltran avatar Aug 20 '24 14:08 miquelbeltran

Hi @miquelbeltran!

I guess the main question is whether such functionality should live in the Dart SDK, or in a package. And if it lives in a package, whether it should be a different package than native_assets_cli: https://pub.dev/documentation/native_assets_cli/latest/native_assets_cli/Architecture-class.html

Also, if we do Architecture, we should also talk about OS. We also don't have an OS in dart:. And the Platform can only talk about the current OS. package:platform also doesn't have an OS, but it can use FakePlatform to talk about a different OS. Because package:platform is already the way to deal with OS (not the cleanest, but okay), I don't think we'll ever add OS to dart: (I believe I opened an issue about this years back, but i can't find it anymore.)

Pros for package:

  • Smaller SDK
  • Easier to iterate
  • OS will also not be in dart:

Pros for dart:

  • No rolling when Abis are added

I think I'm leaning towards a package instead of dart:.

Even though package:native_assets_cli is meant for writing build hooks, nothing prevents you from using that package for the Architecture class in other contexts.

dcharkes avatar Aug 22 '24 09:08 dcharkes

Hi Daco!

My pros for having that in the SDK itself is discoverability and one less 3rd party dependency in code.

native_assets_cli is currently listed as experimental, so I was a bit wary about adding it to my project.

Maybe the team would be open to extract the Architecture and OS parts from it and put it in a separate package under dart-lang/native/pkgs?

miquelbeltran avatar Aug 22 '24 13:08 miquelbeltran

native_assets_cli is currently listed as experimental, so I was a bit wary about adding it to my project.

I don't believe we'd change the API of that class any time. (We are however going to change the package into package:hook.)

Maybe the team would be open to extract the Architecture and OS parts from it and put it in a separate package under dart-lang/native/pkgs?

That's an option. Something like package:native_core, or even package:native. Where we move the most central concepts. But I don't want to do that right now while many things are in flux. While these packages are under development every release cycle requires publishing them in sequence, and it would add yet another step.

dcharkes avatar Aug 22 '24 13:08 dcharkes