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

Add support for specifying a compiler separately from a platform #1776

Closed
natebosch opened this issue Oct 19, 2022 · 10 comments · Fixed by #1903
Closed

Add support for specifying a compiler separately from a platform #1776

natebosch opened this issue Oct 19, 2022 · 10 comments · Fixed by #1903
Labels
type-enhancement A request for a change that isn't a bug

Comments

@natebosch
Copy link
Member

As we look to add support for compiling to wasm and running the browser, we should add a --compiler flag that allows specifying one or more compilers to use in combination with the specified platforms. Using a separate flag will allow using the wasm compiler with any browser that supports the required wasm features.

We'll add a wasm term similar to the js term, for use in @TestOn configuration.

When no --compiler flag is passed, each platform will have a default compiler. When a --compiler flag is passed, only the valid combinations of compiler ✕ platform will run. So a combination like --compiler dart2js --platform vm will run no tests, since the VM platform does not support dart2js as a compiler, but --compiler jit,dart2js,wasm --platform vm,chrome would run both dart2js and wasm tests on chrome, as well as from-source tests on the VM.

In the future, we should be able to add support for compiling with DDC (monolithic compile with the frontend server, it's unlikely we'd implement modular compiles), and may consider adding support for AOT compiles on the VM platform.

@natebosch natebosch added the type-enhancement A request for a change that isn't a bug label Oct 19, 2022
@natebosch
Copy link
Member Author

natebosch commented Oct 19, 2022

This will require a change in the interface between the test runner, and each Platform implementation. In particular, platforms will need to

  • support an argument specifying which compilers to use
  • know which compilers are valid for the platform
  • have a default compiler when no compilers config is specified

cc @christopherfujino - heads up that this will impact integration with flutter_test, likely sometime in Q1. Does the flutter test setup have any notion of running with different compilation approaches on the same platform today?

jakemac53 added a commit that referenced this issue Oct 27, 2022
- Adds the experimental platform `experimental-chrome-wasm`. This requires chrome beta and a very recent SDK to run.
  - This platform will be deleted eventually, and replaced with a [--compiler](#1776) flag.
- Adds `wasm` to the boolean selector variables, and an `isWasm` field to `Runtime` (defaults to false, so should be non-breaking).

Note that the integration test for this will initially be failing, as there is one more change needed from the SDK, but once that lands we can update this to point at the new location for the wasm runtime, update the min SDK, re-run the tests, and land.
@jakemac53
Copy link
Contributor

jakemac53 commented Feb 3, 2023

I have been looking into this a bit, there are a few questions we will want to resolve that I have ran into:

  • In precompiled mode today we allow VM tests to not actually be precompiled to kernel, and in that case we run them from source. This differs from normal tests which we always compile to kernel by default.
    • I think we should change this to compile them to kernel on the fly, if not precompiled (assuming we are in --compiler kernel mode)?
    • Note that build_vm_compilers is no longer supported, so I think we need a fallback. Even though it is a bit weird .
    • We might need to update some stuff internally to pass --compiler none, but that should be doable.
    • We could alternatively not do this and require users to pass --compiler none in precompiled mode, if they aren't precompiling their vm tests to kernel, but that is fairly breaking I think.
  • I am realizing that the behavior of skipping tests for a given platform if no compiler for that platform is specified is potentially a bit weird/cumbersome.
    • For instance, if I want to use wasm for web tests, but also run vm tests, I will have to explicitly specify --compiler kernel for the vm tests.
    • Should we instead just use the default compiler for the platform, if no compatible compiler is given?
    • Note that the error message here is pretty reasonable - it lists the platforms which were specified but matched no tests. I think it would be pretty feasible to make it also list the supported compilers if the given compilers don't overlap it at all.
    • Doing this would also make adding support for new compilers potentially breaking (tests might now run on that compiler instead of the default one).
  • The --use-data-isolate-strategy flag is a bit weird. It should probably imply --compiler none, but the way I have things set up at the moment each runtime defines a default compiler, and it can't change on the fly. Implying the --compiler none flag would make web tests fail since they don't support no compiler (unless we made the above change to have it choose its default in that case).
  • The --pub-serve flag is also a bit weird with this, it has similar issues to --precompiled. For vm tests it doesn't support kernel at all today, and just assumes running from source. I probably want to make this also require the none compiler flag explicitly. For web tests it requires precompiled javascript and doesn't support wasm.
  • Both the --precompiled and --pub-serve flags are weird around distinguishing which javascript compiler was used. We could inspect the actual JS to try and sniff what compiler was actually used but it seems a bit much. It isn't clear what we should do there (we could just trust it).

@jakemac53
Copy link
Contributor

Another question, should we add a special all compiler? It would run with all supported compilers on all platforms. Only makes sense to provide on its own.

@natebosch
Copy link
Member Author

natebosch commented Feb 3, 2023

  • I think we should change this to compile them to kernel on the fly, if not precompiled

I think it makes sense to avoid doing any compilation ourselves in precompiled mode. We should disallow --compiler flags in that mode, and we should ask the platform to run exactly what was precompiled, including letting the VM run from source if that is what was requested.

I'm not sure if that introduces difficulty with the browser platforms - would they need to set things up different based on detecting a precompiled wasm file over a precomiled js file?

  • Should we instead just use the default compiler for the platform, if no compatible compiler is given?

This seems sensible to me, and I like that it sidesteps the other problem below.

  • Doing this would also make adding support for new compilers potentially breaking

I think it's specifically breaking to add support for an existing compiler to an existing platform. So we'd need to know whenever adding a compiler which existing platforms will ever support it, and when adding a platform which existing compilers it will ever support. We can add brand new compilers supported by existing platforms, and we can add new platforms which support existing compilers. I think the biggest risk there is the strangeness with flutter already using DDC and test not supporting it yet, so the rollout could cause us to want to add an existing compiler to an existing platform....
Maybe I'm talking myself out of this plan.

  • Both the --precompiled and --pub-serve flags are weird around distinguishing which javascript compiler was used. We could inspect the actual JS to try and sniff what compiler was actually used but it seems a bit much. It isn't clear what we should do there (we could just trust it).

Yeah, I think we should leave it up to trust. If you told us that you handled compilation, we just run the platform and hope that you did indeed compile to something supported by the platform.

  • The --use-data-isolate-strategy flag is a bit weird.

Dropping this flag in favor of --compiler none or --compiler source would be OK with me. We will likely have a major revision soon to drop the export of matcher, so we could have the flag imply --compiler none for now (with a warning) and drop it in the next major version.

  • Implying the --compiler none flag would make web tests fail since they don't support no compiler (unless we made the above change to have it choose its default in that case)

Brainstorming: You mentioned the idea of using --<platform>-compiler. We could internally implement as if the UX was the --<platform>-compiler flags, but what the user sees is our original plan for --compiler. We would as early as possible translate the --compiler into the equivalent --<platform>-compiler flags (would need to expose Platforms and the notion of their supported compilers to that part of the system, probably OK to do). We could always choose to expose the --<platform>-compiler options (mandate that you pick one way to pass them per invocation maybe). This way the --use-data-isolate-strategy can correctly translate to --vm-compiler source.

@jakemac53
Copy link
Contributor

I think the biggest risk there is the strangeness with flutter already using DDC and test not supporting it yet, so the rollout could cause us to want to add an existing compiler to an existing platform....

Right, I want to also add AOT support to the VM platform. Both AOT and wasm are examples of things that would have broken us, as they are both new compilers that were added to known platforms.

Yeah, I think we should leave it up to trust. If you told us that you handled compilation, we just run the platform and hope that you did indeed compile to something supported by the platform.

SGTM

I'm not sure if that introduces difficulty with the browser platforms - would they need to set things up different based on detecting a precompiled wasm file over a precomiled js file?

It is more than just this - you could have pre-compiled files for multiple compilers present in the precompiled dir. So I think we want to keep the --compiler flag for precompiled mode, and based on the compiler select which files we will execute. It becomes basically the "expected compiler" in a sense.

In other words, I would expect -p vm -c source -c kernel --precompiled <some-dir> to run my tests both from source and from precompiled kernel.

Brainstorming: You mentioned the idea of using --<platform>-compiler. We could internally implement as if the UX was the --<platform>-compiler flags, but what the user sees is our original plan for --compiler. We would as early as possible translate the --compiler into the equivalent --<platform>-compiler flags (would need to expose Platforms and the notion of their supported compilers to that part of the system, probably OK to do). We could always choose to expose the --<platform>-compiler options (mandate that you pick one way to pass them per invocation maybe). This way the --use-data-isolate-strategy can correctly translate to --vm-compiler source.

I do in general like the idea of something like this, I can't decide if I like --<platform>-compiler=<compiler> or --compiler=[<platform>:]<compiler> format more? It feels nice to not have the command line arguments be dynamic, but ultimately it would not be too hard to support.

@natebosch
Copy link
Member Author

Both AOT and wasm are examples of things that would have broken us, as they are both new compilers that were added to known platforms.

I don't think that use case would break. New compilers on known platforms should be fine - no existing scripts would have a working --compiler wasm flag until that compiler is supported, and that flag is required to see the changing behavior when an existing platform picks up support for a new compiler (and stops using the default). So no successful test run could start getting different behavior upon publish. But we would break things if firefox doesn't add support for wasm in that release, and wants to add it later.

In any case, it's likely moot since I think we're moving in the direction of using boolean selector to more flexibly choose compilers and cover the situation of using --compiler browser:wasm without impacting VM platform, assuming it's not too complex to implement.

you could have pre-compiled files for multiple compilers present in the precompiled dir. So I think we want to keep the --compiler flag for precompiled mode, and based on the compiler select which files we will execute. It becomes basically the "expected compiler" in a sense.

I'm not sure that I agree. It seems like --precompiled should just run exactly what is precompiled, and not force the caller to repeat information about what what compiled.
On the other hand I can see the benefit to not forcing the caller to keep the compiled output directory clean of old compilation artifacts, and we do allow filtering out by path instead of requiring that the caller compile only the specific suites that should be run...

I'm ok with treating the flag as a filter in precompiled mode for the reasons you state. Will we plan on having some kind of warning if you pass --compiler dart2js,ddc --precompiled?

@jakemac53
Copy link
Contributor

I'm not sure that I agree. It seems like --precompiled should just run exactly what is precompiled, and not force the caller to repeat information about what what compiled.

But we have to know which precompiled files to run. These aren't just old artifacts they might all be valid for the current run.

I'm ok with treating the flag as a filter in precompiled mode for the reasons you state. Will we plan on having some kind of warning if you pass --compiler dart2js,ddc --precompiled?

That is an interesting case because we expect the same JS files :(. We could potentially change it though, to expect different entrypoint JS files per compiler?

But we don't have any build system today that would support compiling the same app with two different compilers and running those both via a single test.

I am not sure if we want to actually hard code a warning or not.

@natebosch
Copy link
Member Author

We could potentially change it though, to expect different entrypoint JS files per compiler?

But we don't have any build system today that would support compiling the same app with two different compilers and running those both via a single test.

If we did come up with a standard for using a different extension for DDC tests we could probably support it without too much work. I still think it would be nice in general to have separate intermediate files between dart2js and ddc and the copy the entrypoint as the last step, so that we avoid full recompile when switching compilers.

@jakemac53
Copy link
Contributor

If we did come up with a standard for using a different extension for DDC tests we could probably support it without too much work. I still think it would be nice in general to have separate intermediate files between dart2js and ddc and the copy the entrypoint as the last step, so that we avoid full recompile when switching compilers.

Only dart2js actually ends up having to recompile, but this is fair. I worry a bit about just copying it due to the potential size of dart2js files though.

Maybe what I would do if I could go back in time (or do a breaking change?) would be to have people use some sort of magic template identifier to load the JS, instead of hardcoding a *.dart.js path. Maybe something like *.dart.{{compiler}}.js. Then we could have separate outputs, and even compile with both dart2js and DDC at the same time.

@jakemac53
Copy link
Contributor

jakemac53 commented Feb 6, 2023

We should also consider if we should add an @Compiler metadata annotation, to change the default compiler for a given test. This could just be added later though, but I could imagine valid reasons for wanting it.

jakemac53 added a commit that referenced this issue Feb 14, 2023
Closes #1776

## Overview

- Adds a  user facing `--compiler` flag, which can be used to configure which compiler is used to run tests.
- Adds a new `Compiler` class, similar to the `Runtime` class, but it doesn't support custom ones (at least for now).
  - This is exported from `package:test_api/backend.dart`
- Adds required `defaultCompiler` and `supportedCompilers` fields to `Runtime`. This is breaking for custom runtimes.
  - Platforms will not be asked to load tests for compilers that the associated runtimes don't support.
- Add `compiler` field to `SuitePlatform`. This is how platforms get access to compiler configuration. It means there is no API breaking change for platforms, but they do need to be updated to support the configuration.
- Adds support in all platforms to respect the compiler option.
  - The vm platform supports the `source` compiler which is roughly equivalent to `--use-data-isolate-strategy`. The `--use-data-isolate-strategy` flag is now just an alias for `--compiler source` and it it deprecated/hidden.
    - We would call this compiler `none` but we already have `OS.none`, so it would conflict in boolean selectors. 
- Adds a `CompilerSelection` class similar to `RuntimeSelection`, used by `SuiteConfig` to track the selected compiler(s).
- Adds a new `compilerSelections` field to `SuiteConfig`.
- Adds the compilers to the valid boolean selectors, this makes them usable in `TestOn` etc.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants