search

dart-lang / native

Dart packages related to FFI and native assets bundling.
BSD 3-Clause "New" or "Revised" License
157 stars 44 forks source link

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

Open mkustermann opened 2 days ago

mkustermann commented 2 days ago

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 commented 2 days ago

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 commented 2 days ago

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 commented 2 days ago

.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 commented 2 days ago

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)