Skip to content

[native_assets_cli] Remove OS and architecture from CodeAsset #2127

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.

Sign up for GitHub

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

Jump to bottom
Closed
dcharkes opened this issue Mar 24, 2025 · 2 comments
Closed

[native_assets_cli] Remove OS and architecture from CodeAsset #2127

dcharkes opened this issue Mar 24, 2025 · 2 comments
Assignees
@dcharkes
Labels
package:hooks
Milestone
Native Assets v1.0

Comments

@dcharkes
Copy link
Collaborator

CodeAssets occur in an input or an output belonging to an input. We could simply remove the OS and architecture from the assets, because they must have the OS/architecture that is in the code config.
@dcharkes dcharkes added this to the Native Assets v1.0 milestone Mar 24, 2025
@dcharkes dcharkes mentioned this issue Mar 24, 2025
Merged
@dcharkes
Copy link
Collaborator Author

Notes from discussion with @mkustermann:

  • Do we need to consider future support for multi-architecture code assets before removing the OS/arch as output?
  • If we want to ever add support for fat binaries, we'd probably want to add that as a MultiArchCodeAsset with a MultiArchCodeConfig
    • Which would have multiple architectures in the config.
    • The multi_architecture_code_asset should be listed
    • Must the output contain all requested architectures? Or only a subset?
      • If a subset, then it must be listed in the output.
    • It would be a separate hook invocation.
    • Open questions:
      • How would that work together with the normal code assets? Can these be mixed and matched? Would Flutter for example combine a fat binary with two architectures and and a non-fat-binary with a single architecture if they have the same asset id?
  • These MultiArchitectureCodeAssets would likely live in the same package:code_assets such that they can share OS, Architecture and friends.

auto-submit bot pushed a commit that referenced this issue Mar 26, 2025
@dcharkes
[native_assets_cli] Validate conditionally required fields (#2126)
4073532 
Closes: #1826

This PR adds the final step to the generated syntax validation: conditionally required fields:

1. If target OS is `x`, then require `x` config in code config.
2. If target OS is `windows`, then require `windows` in the c compiler config (more info #1913).
3. If link mode is dynamic library bundled or static library, then require a file in a code asset.

We could consider trying to nest the fields under the condition, but that has other downsides:

RE 1: Then the OS is no longer an enum usable in the code-asset as OS field. (We could consider this if we remove the OS/arch from code asset outputs. We should be able to do this due to the code config always having a single OS and architecture anyway. #2127)
RE 2: That would mean the compiler config would be split over two places. `input.config.code.cCompiler` and `inputconfig.code.windows.cCompiler`. Maybe that's better? Maybe not?
RE 3: Treating a group of files in assets would then become `input.assets.code.switch( ... )` instead of simply `input.assets.code.map((a) => a.file)`. Maybe that's okay because we don't often use files in such way anyway?

WDYT @mosuem @HosseinYousefi?

(I'd probably do any of those refactorings in follow up PRs.)
@dcharkes dcharkes mentioned this issue Mar 26, 2025
Merged
@dcharkes dcharkes moved this to In Progress in Native Assets Mar 26, 2025
@dcharkes dcharkes self-assigned this Mar 26, 2025
auto-submit bot pushed a commit that referenced this issue Mar 27, 2025
@dcharkes
[native_assets_cli] Remove CodeAsset os and architecture getters (
627484f 
#2138)

Bug: #2127

We want to remove these fields as they are redundant with the `CodeConfig`.

This PR does not yet change anything in the protocol, but removes all the uses of the fields.

When rolling this PR into Dart and Flutter, we'll need take the OS and architecture from the `CodeConfig` passed into a hook build.
@dcharkes dcharkes mentioned this issue Mar 31, 2025
Merged
github-merge-queue bot pushed a commit to flutter/flutter that referenced this issue Apr 2, 2025
@dcharkes
[native_assets] Roll dependencies (#166282)
9aa366f 
Updating the dart-lang/native dependencies to the ones published today.

No functional changes, but `CodeAsset` does not expose an `architecture`
and `os ` anymore (dart-lang/native#2127).
Instead these should be taken from what is passed in for the
`CodeConfig`. This PR refactors the `DartBuildResult` to carry around
the `Target` with `CodeAsset`s as `FlutterCodeAsset`s.

(This PR avoid refactoring relevant code due to
#164094 already refactoring this
code.)
github-merge-queue bot pushed a commit to flutter/flutter that referenced this issue Apr 2, 2025
@dcharkes
[native_assets] Roll dependencies (#166282)
597d569 
Updating the dart-lang/native dependencies to the ones published today.

No functional changes, but `CodeAsset` does not expose an `architecture`
and `os ` anymore (dart-lang/native#2127).
Instead these should be taken from what is passed in for the
`CodeConfig`. This PR refactors the `DartBuildResult` to carry around
the `Target` with `CodeAsset`s as `FlutterCodeAsset`s.

(This PR avoid refactoring relevant code due to
#164094 already refactoring this
code.)
github-merge-queue bot pushed a commit to flutter/flutter that referenced this issue Apr 3, 2025
@dcharkes
[native_assets] Roll dependencies (#166282)
dec31cb 
Updating the dart-lang/native dependencies to the ones published today.

No functional changes, but `CodeAsset` does not expose an `architecture`
and `os ` anymore (dart-lang/native#2127).
Instead these should be taken from what is passed in for the
`CodeConfig`. This PR refactors the `DartBuildResult` to carry around
the `Target` with `CodeAsset`s as `FlutterCodeAsset`s.

(This PR avoid refactoring relevant code due to
#164094 already refactoring this
code.)
@dcharkes
Copy link
Collaborator Author

The fields are no longer being red by newer hooks/SDKs.

They are still written for backwards compatibility, we can clean this up in a couple of weeks.

@github-project-automation github-project-automation bot moved this from In Progress to Done in Native Assets Apr 14, 2025
@dcharkes dcharkes mentioned this issue Apr 22, 2025
Merged
auto-submit bot pushed a commit that referenced this issue Apr 22, 2025
@dcharkes
[native_assets_cli] Cleanup JSON: Code Asset OS and Architecture (#2219)
4e1ef75 
Bug:

* #2199

Cleanup from:

* #2127
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dcharkes dcharkes

Labels
package:hooks
Projects
Native Assets
Status: Done
Milestone
Native Assets v1.0
Development

No branches or pull requests
1 participant
@dcharkes