[dart:io] Adds Platform.architecture #56959
Conversation
|
Thank you for your contribution! This project uses Gerrit for code reviews. Your pull request has automatically been converted into a code review at: https://dart-review.googlesource.com/c/sdk/+/391306 Please wait for a developer to review your code review at the above link; you can speed up the review if you sign into Gerrit and manually add a reviewer that has recently worked on the relevant code. See CONTRIBUTING.md to learn how to upload changes to Gerrit directly. Additional commits pushed to this PR will update both the PR and the corresponding Gerrit CL. After the review is complete on the CL, your reviewer will merge the CL (automatically closing this PR). |
Exposes "Platform::HostArchitecture" from the runtime to the "dart:io" package as a new getter under "Platform". This could be useful for determining which CPU architecture Dart is running on.
RossComputerGuy
force-pushed
the
feat/target-arch
branch
from
d5c1a98 to
905760b
Compare
|
https://dart-review.googlesource.com/c/sdk/+/391306 has been updated with the latest commits from this pull request. |
1 similar comment
|
https://dart-review.googlesource.com/c/sdk/+/391306 has been updated with the latest commits from this pull request. |
pkg/dart2native/lib/generate.dart
Outdated
| /// Specifies the CPU architecture the executable is being generated for. This | ||
| /// must be provided when [_kind] is [Kind.exe], and it must match the current | ||
| /// CPU architecture. | ||
| final String? _targetArch; |
There was a problem hiding this comment.
According to Dart Doc, the first line should be brief: https://dart.dev/effective-dart/documentation#do-separate-the-first-sentence-of-a-doc-comment-into-its-own-paragraph
Doing as suggested will improve the generated API documentation (dart doc) experience.
| /// Specifies the CPU architecture the executable is being generated for. This | |
| /// must be provided when [_kind] is [Kind.exe], and it must match the current | |
| /// CPU architecture. | |
| final String? _targetArch; | |
| /// Specifies the CPU architecture the executable is being generated for. | |
| /// | |
| /// This must be provided when [_kind] is [Kind.exe], and it must match the current | |
| /// CPU architecture. | |
| final String? _targetArch; |
There was a problem hiding this comment.
Alright, I wasn't aware of that since I based it on _targetOS for the doc comment.
There was a problem hiding this comment.
I've updated the comment.
| @@ -182,6 +191,13 @@ class _Generator { | |||
| _sourcePath = _normalize(sourceFile)!, | |||
| _packages = _normalize(packages) { | |||
| if (_kind == Kind.exe) { | |||
| if (_targetArch == null) { | |||
| throw ArgumentError('targetArch must be specified for executables.'); | |||
There was a problem hiding this comment.
I believe the Flutter team once went over the errors the tooling could throw and added clear explanation points on how to move forward with the aim of improving the developer experience. I would expect such practice to also be present here.
If I encounter "targetArch must be specified for executables" I would ask myself:
- "what is targetArch", is it an abbreviation for "target architecture"? (see Effective Dart for abbreviations)
- How can I specify so? Is it a parameter option
--targetArch? Is there an example on how to specify it? - What are the available architectures I can compile into? Is there documentation stating all the supported architectures? Maybe we can point the reader to: https://github.com/dart-lang/sdk/blob/main/docs/Supported-Architectures.md? Note that these are already in the "help command with TargetArch.names, but it is not as clear as the table provided in the documentation.
- What architecture is my system running? Could the message include a suggested resolution with the current architecture my system is using?
For a great developer experience, I would hope all the above questions are answered within the error message I encounter.
For example, a new improved message would be (ignore bad formatting and bad command):
ArgumentError('''
The target architecture for the Dart executable must be specified and it was not. Make sure you specify
the `--targetArch` option to the compile command:
dart compile exe bin/foo.dart --target-os macos --target-arch armv8
Refer to [supported architectures](https://github.com/dart-lang/sdk/blob/main/docs/Supported-Architectures.md)
for a comprehensive table of all the available architectures per operating system.
''');I would also like to get a message on how to request support for an architecture that is not already supported, so if the input is for an unsupported os-architecture combination point me to the repository where I could open an issue to request support or contribute back.
There was a problem hiding this comment.
"what is targetArch", is it an abbreviation for "target architecture"?
Yes, it is an abbreviation for "target architecture". I was thinking targetCPU but in my experience, that usually is for specific CPU optimizations (eg. zen1, skylake, generic, baseline).
How can I specify so? Is it a parameter option
--targetArch? Is there an example on how to specify it?
--target-arch was added to pair with --target-os. However, --target-os doesn't support cross compiling and so I haven't been able to get --target-arch to cross compile. I at least added the logic for --target-arch to pair with the existing --target-os flag.
What are the available architectures I can compile into? Is there documentation stating all the supported architectures? Maybe we can point the reader to: https://github.com/dart-lang/sdk/blob/main/docs/Supported-Architectures.md? Note that these are already in the "help command with TargetArch.names, but it is not as clear as the table provided in the documentation.
The TargetArch.names I added in pkg/vm/lib/target_arch.dart was based on that table + observations I saw when using grep to figure out how this repo works.
What architecture is my system running? Could the message include a suggested resolution with the current architecture my system is using?
Could you provide more information to what this sort of means?
For example, a new improved message would be (ignore bad formatting and bad command
Should we also change the one for the target OS?
RossComputerGuy
force-pushed
the
feat/target-arch
branch
from
905760b to
e15d869
Compare
|
https://dart-review.googlesource.com/c/sdk/+/391306 has been updated with the latest commits from this pull request. |
1 similar comment
|
https://dart-review.googlesource.com/c/sdk/+/391306 has been updated with the latest commits from this pull request. |
|
If we do add this to the platform libraries, I'd put it in The usual caveat about exposing information applies: You can always provide more detailed information, until you have made a complete API for querying What is an "architecture"? Will any answer we give here actually be enough for users? (And for what?) Nothing new here. We have the same issue with |
Ime, it's always referring to the actual ISA of the CPU.
It reports what the host is. I think the closest example to look at is Node.js's
I think it depends, if it's an ffi related use then the abi should be used to determine the architecture. This property could be useful when a program wishes to download a binary and it depends on the architecture or maybe like a compiler where ffi isn't used inside the compiler.
Definitely, it's hard to determine what exactly every use case is. |
|
https://dart-review.googlesource.com/c/sdk/+/391306 has been updated with the latest commits from this pull request. |
Platform.architectureintodart:io, this could be useful to easily detect the CPU architecture--target-archto pair with the--target-osflag, maybe someday these two can support full cross compilationContribution guidelines:
dart format.Note that this repository uses Gerrit for code reviews. Your pull request will be automatically converted into a Gerrit CL and a link to the CL written into this PR. The review will happen on Gerrit but you can also push additional commits to this PR to update the code review.