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

[native_assets_cli] BuildConfig BuildOutput hierarchy #12

Closed
dcharkes opened this issue Apr 20, 2023 · 4 comments · Fixed by #30
Closed

[native_assets_cli] BuildConfig BuildOutput hierarchy #12

dcharkes opened this issue Apr 20, 2023 · 4 comments · Fixed by #30
Labels
P2 A bug or feature request we're likely to work on

Comments

@dcharkes
Copy link
Collaborator

dcharkes commented Apr 20, 2023

Isn't this more general than only for native assets?

{
  'out_dir' :  ...,
  'native_assets' : {
    'c_compiler' : {
      'cc' : ...
      'cflags' : ...
      'ld' : ...,
    },
    'rustc' : {
      '...' : ...
    },
    'link_mode_preference' : 'static',
    ...
   },
}

I'd make this more hierarchical

Originally posted by @mkustermann in #3 (comment)

@dcharkes
Copy link
Collaborator Author

dcharkes commented Apr 20, 2023

This is an interesting idea.

In the discussions around build.dart earlier we discussed that dartdev/flutter_tools can start providing more config for different types of assets. I asked whether, package:native_assets_cli should not be package:build_cli instead and do all config [1]. That idea was rejected to keep the configs separate. But even if the config separate, it could indeed be useful to not pollute the namespace in the config file.

For build-output the same type of splitting of concerns yields some interesting questions.

  • Single build output file?
    • The yaml from both packages BuildOutput would need to be merged into a single yaml.
      • This actually is kind of an argument to make a separate package package:build_cli that has the full build.dart supported spec.
    • would need to merge dependencies?
    • would need to decide on one time-stamp when the built was run
  • Multiple output files
    • how do dartdev/flutter_tools then decide whether to run it again?
      • they would need to inspect all output files and check the dependencies and timestamps of all of them.
    • doesn't really work because if you have both native assets and icons built by the same built.dart but using two different packages (package:native_assets_cli and package:icons_cli), they would need to merge their json/yaml.

[1] internal meeting notes "package:native_assets_cli doesn’t have to be package:build_cli, for other types of assets we can have a different CLI spec."

@dcharkes dcharkes changed the title Isn't this more general than only for native assets? Should BuildConfig BuildOutput also work for other asset types later? Apr 20, 2023
@dcharkes
Copy link
Collaborator Author

@natebosch @jakemac53 Maybe we should revisit if the CLI spec should be tied to Native Assets only. When think we might add other types of assets, for example icons, later.

@jakemac53
Copy link

Maybe consider just versioning this config file? Then you don't have to over-generalize it for now, but we would have the ability to later.

@dcharkes dcharkes changed the title Should BuildConfig BuildOutput also work for other asset types later? BuildConfig BuildOutput hierarchy Apr 28, 2023
@dcharkes dcharkes changed the title BuildConfig BuildOutput hierarchy [native_assets_cli] BuildConfig BuildOutput hierarchy Apr 28, 2023
@dcharkes dcharkes added the P2 A bug or feature request we're likely to work on label Apr 28, 2023
@dcharkes dcharkes mentioned this issue Apr 28, 2023
@dcharkes dcharkes changed the title [native_assets_cli] BuildConfig BuildOutput hierarchy [native_assets_cli] BuildConfig BuildOutput hierarchy Apr 28, 2023
@dcharkes
Copy link
Collaborator Author

dcharkes commented May 1, 2023

I believe nesting the c_compiler config is useful. With the Windows toolchain we also require a script that sets environment variables and the arguments to that script. That means we have 5 vars already related to the C compiler.

Nesting in native_assets could be useful because it would isolate the config for native assets from configs passed to other types of assets. However, it causes very long environment variables: NATIVE_ASSETS__C_COMPILER__CC vs C_COMPILER__CC. We're using these environment variables for example on the Dart CI where the c compiler is not installed in the default locations. Currently, the only key that would be under native_assets besides the c_compiler is link_mode_preference.

edit: I'll address this now, so that we don't have to major version rev this later. (#24)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 A bug or feature request we're likely to work on
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants