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

Discussion: Dart Codegen and Platforms #1436

Closed
jakemac53 opened this issue May 14, 2018 · 8 comments
Closed

Discussion: Dart Codegen and Platforms #1436

jakemac53 opened this issue May 14, 2018 · 8 comments

Comments

@jakemac53
Copy link
Contributor

jakemac53 commented May 14, 2018

See #1430 and dart-lang/sdk#32960 for context.

As we expand support for build_runner to support non-web customers as first class clients, we need to figure out how users will write and run Builders that require certain platforms, and how we will provide support for that within build_runner and bazel_codegen.

I envision in the short term essentially two classes of Builder:

  • Platform agnostic (json_serializable, inject, etc):
    • These builders should be able to build to source, which means shipping the generated code with the package, and it should be cross-platform compatible.
    • Should be able to run within any platform specific scope (can't crash because of a dart:ui import).
      • But doesn't necessarily need to give you access to types from those imports.
  • Platform specific (angular, likely future fuschia/flutter targeted codegen):
    • These must be able to resolve types from the platform they are targeting (dart:html, dart:io, dart:ui).
    • Not required to be able to run on other platforms.

To solve both of these use cases, I propose that we add a platform field to the builder definition in build.yaml. This would take a single value, which is either a platform identifier such as web (angular would do this), or all. For now at least, we would not allow you to support multiple explicit platforms.

If you opt into a specific platform in your builder, then we would ensure that the Resolver you get is specific to that platform. Otherwise, we would either use a new core_libs platform or in absence of that simply choose a canonical platform (likely vm, although this really isn't ideal).

In the presence of config specific imports, platform agnostic builders would get the default import, while platform specific builders would get the import resolving to their platform, if available.

This enables "safe" cross-platform builders by not exposing much from any specific platform to cross-platform generated code (you would still have some constants most likely, and I am sure a few other things, but the hypothesis is that its unlikely to cause real world problems). This would create a limitation on what platform agnostic builders can do potentially, but the use cases should be narrow and there should be at least one workaround available, which is to use config specific imports with stubs in the default import.

This also enables platform specific builders to ensure they can access everything from the platform they are targeting.

Note that this proposal would almost certainly be blocked on using summary/kernel based Resolvers implementation (in build_runner) to make it sufficiently cheap to create multiple analysis contexts.

cc @sigmundch @leafpetersen @natebosch @grouma @dgrove @matanlurey

@jakemac53
Copy link
Contributor Author

@davidmorgan as well because this also has implications for bazel_codegen and combining of tasks - we would likely only be able to group tasks that specify the same platform (or at least the overhead incurred by not doing so would outweigh the benefits).

@jakemac53
Copy link
Contributor Author

One possibly amendment/optimization for this proposal, would be to change the meaning of all to select a platform from the already depended on platforms (from other builders) in a deterministic way.

There would be a few advantages to this:

  • Fewer kernel files (one less parallel tree potentially).
  • Platform specific constants would actually be available, for the automatically selected platform.

I see one primary disadvantage here, which is subtle changes in your package could change the platform that the platform agnostic builders end up getting. We could allow an override in some way, possibly for the builder configuration or possibly even adding the platform field to the target configuration.

@natebosch
Copy link
Member

But doesn't necessarily need to give you access to types from those imports.

I'm not sure of this. I think certain things like the name of the type are likely to be required for platform agnostic codegen.

Note that this proposal would almost certainly be blocked on using summary/kernel based Resolvers

Which summaries or kernel files do we use for platform agnostic codegen?

@jakemac53
Copy link
Contributor Author

I'm not sure of this. I think certain things like the name of the type are likely to be required for platform agnostic codegen.

I think for most cases this won't matter, and I also don't see a way to avoid the restriction other than the options I have listed above:

  • Add a "platform" config to the "target" definition, in which case platform agnostic builders would get that platform. We could also infer platforms similarly to how we do in bazel (based on target deps) this way.
  • Infer a platform if only one platform is supported by all builders used by the package.

Which summaries or kernel files do we use for platform agnostic codegen?

Same answer - use ideally some restricted platform summary (core_libs?) if the target being applied to supports all platforms. If it supports only a single platform then use that platforms summary.

@jakemac53
Copy link
Contributor Author

cc @vsmenon as well

@natebosch
Copy link
Member

After some more brainstorming we have a few approaches to consider.

Static configuration

This is essentially the above proposal optionally combined (or superseded) by configuration at the target level alongside (or instead) of the builder level. By allowing configuration on the target we would solve the case of resolving specific types from dart:ui or dart:html by a Builder that is platform agnostic.

Pros

  • Looks a lot like our most likely solution for bazel. We already have a per-target notion of platform and we could us that to drive a decision about which SDK to use for codegen. Targets may have multiple platforms but we should be able to pick any of them in those cases.
  • Maintain a single canonical version for a file in the build system so things like the analyzer can know exactly what to use.

Cons

  • I think this is very likely to push a configuration burden on our customers, even for relatively simple cases. We can infer some information about platforms internally, but that's mostly because in bazel we're already required to manually split out sources into targets and specify their dependencies. Based on provided module boundaries we can infer platforms but we haven't proven we can infer both module boundaries and platforms.
  • Does not help with config specific imports.

"Merge" SDK dill files

We might be able to know at the start of a build what the combined set of SDKs are, and it might be possible to take multiple SDK dill files and use them simultaneously. This would take some work in the front_end and we'd need to carefully consider what should be exposed - ie if 2 SDKs have a different value for some constant or for resolved types how should we surface that?

Pros

  • Our builders write Dart source (at least for now) and this matches the way I think about writing Dart source. When I'm writing I don't think about the implementation details of the SDK or how the various platforms might differ other than whether I can import dart:html at all. If I don't think about some details while I'm writing Dart code, can we also hide those details from Builders and therefor have a single notion of the SDK for the parts of the build outputting source code?
  • Would not be an additional configuration burden.
  • Maintain a single canonical version for a file in the build system so things like the analyzer can know exactly what to use.

Cons

  • Could get tricky on the kernel/front_end side.
  • Would fall down if a Builder does start to need details from a specific SDK like a resolved constant.
  • Does not help with config specific imports.

Generate a Dart source file per platform

For any Builder which uses a Resolver we could build an output per platform where each one had a Resolver backed by that platform's SDK. We could force these builders to be lazy so that we only run for the platforms where we need to (requires getting our full kernel pipeline working for the VM so we can lazily build Dart sources for that platform).

Pros

  • Config specific imports have an easy solution in this approach.
  • We can be confident we're generating the correct code with the correct information without risking either using the wrong information or being unable to access some information.

Cons

  • We'd lose the 1 version of a given file notion. Not sure what we'd do with the analysis server (cc @MichaelRFairhurst), and we'd need to either do some manual translations of filenames ourselves before generating kernel, or add some concept of a URI map to the front end so that we could tell it to treat foo.g.dart.vm as if it was foo.g.dart when we're compiling VM flavored kernel.
  • It would mean a lot more build actions for any files which do get used in multiple platforms.
  • We'd need an entirely different solution for to-source codegen.

@MichaelRFairhurst
Copy link
Contributor

Regarding the final option, one thought that comes to mind is that this is already a problem we have inside the analyzer: for instance, the dart2js vs dartium flavors of dart:html. And the vm vs js forms of dart:http. I believe we currently assume they are equally viable and pick one, which may not work here.

we could also use conditional imports, but that requires code generators to write an interface file in addition to a source code per configuration. I'm basing that off of this proposal, which may be out of date and/or differ from the spec though... https://github.com/munificent/dep-interface-libraries/blob/master/Proposal.md

I think in general we would want to shy away from analysis depending upon things that can't be known at analysis time, though. That for instance is what led to the auto const issues.

@jakemac53
Copy link
Contributor Author

Closing this - we now build the SDK summary on the fly from your current sdk which solves the flutter case, which was the primary thing we care about.

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

No branches or pull requests

3 participants