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

Move BuildConfig.targetOS to BuildConfig.codeConfig.targetOS, remove it entirely or extend to cover web #1738

Open
mkustermann opened this issue Nov 20, 2024 · 4 comments

Comments

@mkustermann
Copy link
Member

Currently the CLI protocol mandates BuildConfig.targetOS. Though making builds for the web platform wouldn't have any operating system. So we could either move this to BuildConfig.codeConfig.targetOS or remove it entirely.

We may decide that we introduce a BuildConfig.targetPlatform which is the platform we're going to run Dart code on - but this is different from an operating system concept ("web" is a target platform, but not an operating system).

/cc @dcharkes

@dcharkes
Copy link
Collaborator

remove it entirely

This is not an option right? hook/build.dart needs to know what OS a dylib must be built for.

Move [...] or extend to cover web

BuildConfig.codeConfig.targetOS missing while BuildConfig.codeConfig.targetArchitecture is there is slightly annoying, so moving could make sense.

BuildConfig.targetPlatform

That would be native and web?

If users have data assets that are OS dependent, they might start misusing config.codeConfig.targetOS to emit different icons for Android/iOS. How confident are you that we don't need an OS for data assets?

@mkustermann
Copy link
Member Author

mkustermann commented Nov 20, 2024

Again, the concept of "operating system" does not make sense in general, because for web builds there's no operating system.

The more general concept of "target platform" is a superset, it gives the same information as OS but also allows expressing we compile for the web. So that would allow removing targetOS (as it can be deducted from the more general targetPlatform).

We could structure it slightly differently and have

BuildConfig
  .targetPlatform (if it's native, .nativeConfig will be set, if it's web .webConfig will be set)
  .nativeConfig
     .targetOS
     .codeConfig
         .cCompiler
           .archiver/.linker/....
  .webConfig
     .compiler (JS or Wasm)
     .crossOriginIsolated (or whathever web specific options)

In any case I believe for web builds it doesn't make sense to have OS - except if made some artificial OS.web but that seems very weird because the actual OS the web build runs on a particular operating system, we just don't know at compile-time.

I think we should reduce the top-level scope on BuildConfig to an absolute minimum.

@dcharkes
Copy link
Collaborator

.nativeConfig .webConfig

👍
I like this more than .codeConfig and .dataConfig. Because it feels somewhat weird to duplicate info such as targetOS in both .codeConfig and .dataConfig or say .dataConfig does not get access to to OS because it might not exist on the web.

@mkustermann
Copy link
Member Author

mkustermann commented Nov 20, 2024

I like this more than .codeConfig and .dataConfig. Because it feels somewhat weird to duplicate info such as targetOS in both .codeConfig and .dataConfig or say .dataConfig does not get access to to OS because it might not exist on the web.

I would never introduce .dataConfig. But .codeConfig may still make sense because one can imagine situations where a dart build targets native (e.g. linux) but doesn't support code assets. In which case the bundling tool doesn't have to provide c compiler configuration, target abi levels, etc (basically codeConfig is there or not there and it groups multiple things that are needed when code assets are enabled - otherwise we'd have several fields on .nativeConfig that are either all set or none set depending on whether code assets are enabled)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants