dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.08k stars 1.56k forks source link

Expose `architecture` from `Abi` class #56521

Open miquelbeltran opened 3 weeks ago

miquelbeltran commented 3 weeks ago

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

dart-github-bot commented 3 weeks ago

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.

lrhn commented 3 weeks ago

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.

miquelbeltran commented 3 weeks ago

Even the Dart team does it ;)

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

lrhn commented 3 weeks ago

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.

miquelbeltran commented 3 weeks ago

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.

dcharkes commented 3 weeks ago

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:

Pros for dart:

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.

miquelbeltran commented 3 weeks ago

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?

dcharkes commented 3 weeks ago

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.