Adds support for config specific imports #1793

jakemac53 · 2018-08-27T16:44:42Z

Most of the changes are to the build_modules package, and there is now a separate set of module files per platform.

Each platform is set up with a builder in the build_modules package - but they are not applied by default. Any new platforms should also be configured in the build_modules package.
Individual compiler packages (or other consumers of modules) must apply the module builder for the platforms they care about, using applies_builders config.

cc @pulyaevskiy the build_node_compilers package will need an update as well - see the build_web_compilers package updates here.

… and platform specific pipelines

grouma · 2018-08-27T22:55:34Z

_test/test/common/message_io.dart

@@ -0,0 +1,5 @@
+// Copyright (c) 2017, the Dart project authors.  Please see the AUTHORS file


grouma · 2018-08-27T22:56:08Z

_test/test/config_specific_import_test.dart

+
+import 'package:test/test.dart';
+
+// ignore: uri_does_not_exist


Are we always going to need these ignore comments? That's quite annoying.

See dart-lang/sdk#34177

grouma · 2018-08-27T22:56:36Z

_test/test/config_specific_import_test.dart

+
+main() {
+  test('Message matches expected', () {
+    if (1.0 is int) {


We really need to provide support for this in a better way.

Ya, its really weird how the dart.library.* things aren't real constants just some special identifier thing that compilers know about?

grouma · 2018-08-27T22:57:41Z

build_modules/CHANGELOG.md

+        strategy: fine
+```
+
+  The supported platforms are currently `dart2js`, `dartdevc`, and `vm`.


Why don't we support flutter?

Because it can't be discovered in the same libraries.json file 😢

grouma · 2018-08-27T23:01:06Z

build_modules/lib/src/meta_module_builder.dart

-    var id = AssetId(buildStep.inputId.package, 'lib/$metaModuleExtension');
+    var platformLoader = await buildStep.fetchResource(platformsLoaderResource);
+    var platforms = await platformLoader.load(buildStep);
+    var platform = platforms[_platform];


Can we wrap this logic up in a method? It took a while to parse what was going on.

grouma · 2018-08-27T23:05:39Z

build_modules/lib/src/module_library.dart

+              'dart.library.<name> constants are supported.');
+        }
+        var library = condition.substring('dart.library.'.length);
+        var coreLibSupport = platform.libraries[library];


I'm not sure what's going on here with coreLibSupport.

This is basically just checking if the current platform explicitly supports the core library that is being referenced in the conditional import. It might either omit the library entirely or explicitly have it marked as unsupported.

natebosch · 2018-08-27T23:30:25Z

build_modules/lib/src/platform.dart

+  PlatformsLoader._();
+
+  Future<Platforms> load(AssetReader reader) async {
+    if (_invalidated) {


[optional - not worth a rebase if you planned on merging instead of squashing] invert the condition and bail out early to reduce 1 level of nesting?

natebosch · 2018-08-27T23:35:28Z

build_modules/lib/src/module_library.dart

+        var conditions = <String, AssetId>{r'$default': linkedId};
+        for (var condition in conditionalDirectiveConfigurations) {
+          if (Uri.parse(condition.uri.stringValue).scheme == 'dart') {
+            throw 'Unsupported conditional import of '


Why is this unsupported?

I don't think it has any value (you can't use things from them anyways) - tracking it would have meant tracking sdk deps per platform which complicates things unnecessarily.

you can't use things from them anyways

I don't understand, this seems valid:

import 'fake_io.dart' if (dart.library.io) 'dart:io';

Hmm I guess in that sense you could use it, I hadn't considered the fake io case but I don't think that is a common use case anyways?

My thoughts regarding this were that the different libraries don't have any overlapping types, so directly doing this would not have any value:

import 'default.dart' if (dart.library.io) 'dart:io' if (dart.library.html) 'dart:html';

yeah I think the fake_io, fake_html thing is going to be a real use case...

I agree that there isn't much value in tracking it, and we can probably assume that they are "correct" in that you won't have something like if (dart.library.io) 'dart:html' - and let the failure be late if you do.

Is it OK to ignore them instead of throw here?

I don't actually think the fake_io case would be common though - in what scenario would that actually be a good pattern?

Primarily this feature is meant to be used by libraries I think, where they are going to want some real implementation and not just fall back on a fake. I don't think it is particularly cumbersome to simply force people to come up with a more generic api and have their actual platform specific imports hidden behind a conditional import.

In theory you could use a fake within a test but I don't understand the value that the feature provides there since you are typically only running on one platform or always running with a fake/mock.

natebosch · 2018-08-27T23:37:44Z

build_modules/CHANGELOG.md

+### Improvements
+
+- Modules are now platform specific, and config specific imports are supported,
+  but only for `dart.library.*` constants.


I think this is already an constraint on conditional imports in the CFE (though it's maybe not documented super well?)

I don't think we should phrase this as if it's a limitation. "config specific imports against dart.library.* constants are supported"

though it's maybe not documented super well?

or at all ;)

natebosch · 2018-08-27T23:38:46Z

build_modules/build.yaml

+  dartdevc:
+    import: "package:build_modules/builders.dart"
+    builder_factories:
+      - metaModuleBuilderFactoryForPlatform('dartdevc')


[aside] If we had union types I would totally make the BuilderFactory typedef be Builder|List<Builder> Function()

Yep, would clean this up a bunch

natebosch · 2018-08-27T23:40:14Z

build_modules/CHANGELOG.md

+        strategy: fine
+```
+
+  The supported platforms are currently `dart2js`, `dartdevc`, and `vm`.


Because it can't be discovered in the same libraries.json file 😢

pulyaevskiy · 2018-08-28T01:51:54Z

Thanks for tagging me @natebosch! Will make sure to update when this lands.

jakemac53 · 2018-08-28T15:44:59Z

@natebosch @grouma I am reconsidering using the libraries.json file for a couple reasons, curious what you think.

It is unlikely that a platform changes its dart: library support any time soon
Flutter doesn't ship a libraries.json file in the same place
It makes the tests easier to write (don't have to mock the file)

grouma · 2018-08-28T15:48:38Z

I'm in favor of hard coding the mapping. As you said, it is unlikely that specific imports are added regularly. If that situation changes we can easily adjust the solution. I'd rather reduce the complexity for now.

natebosch · 2018-08-28T17:00:28Z

@vsmenon - can we expect stability in inclusion/exclusion of dart.library.* for each platform throughout the lifetime of Dart 2.x?

I'm OK with hardcoding this if we are reasonably confident that it's not going to allow for incompatibilities against newer SDKs before there is a breaking version bump. I was more nervous about this before stable.

Other than old versions existing which are possible to pub solve against - do we think there are any reasons this isn't an easily reversible choice? We can hardcode for now and add back libraries.json parsing in the future if we decide it's warranted right?

jakemac53 · 2018-08-28T17:10:23Z

We can hardcode for now and add back libraries.json parsing in the future if we decide it's warranted right?

Right, it should be a transparent change.

vsmenon · 2018-08-28T17:32:15Z

can we expect stability in inclusion/exclusion of dart.library.* for each platform throughout the lifetime of Dart 2.x?

@natebosch Yes, I think so. We're not making breaking changes in 2.x here - and removing would clearly be breaking. We might consider new libraries / platform.

We're hardcoding what's true and false in DDC today.

…dart:io conflict

jakemac53 · 2018-08-29T18:36:39Z

Ok, refactored the platform support handling:

hardcoded now instead of read from libraries.json
renamed Platform to DartPlatform to not conflict with dart:io's Platform
most apis now take a DartPlatform instead of a string
only specific top level constants are exposed for platforms, constructor is private

jakemac53 · 2018-08-29T19:35:35Z

Looks like the build_script_generate test is getting flaky (timeouts) - submitting with red appveyor

jakemac53 added 6 commits August 27, 2018 09:24

add support for parsing libraries.json

2116282

plumb platform support through to all builders

37be3e7

update changelog/pubspec/build.yaml

5b2b037

update tests

fc64c7d

update other packages and tests to work with the latest build_modules…

8c6ad07

… and platform specific pipelines

move vm pipeline to build_modules, update changelog in build_modules

d838603

googlebot added the cla: yes Google is happy with the PR contributors label Aug 27, 2018

grouma reviewed Aug 27, 2018

View reviewed changes

natebosch reviewed Aug 27, 2018

View reviewed changes

fix build_modules tests

86e89ca

jakemac53 added 2 commits August 28, 2018 09:08

extract platform loading logic into a method

eecc7ab

code review updates, fix build_web_compilers test

6e34ded

jakemac53 added 4 commits August 28, 2018 12:45

fix expected test numbers in integration_test

5d24573

add new entrypoints to build.dart2js.yaml

bd18154

Merge branch 'master' into config-specific-imports

d20f8e3

hardcode platform support, rename Platform to DartPlatform to remove …

57f0005

…dart:io conflict

natebosch approved these changes Aug 29, 2018

View reviewed changes

jakemac53 merged commit ba5641f into master Aug 29, 2018

jakemac53 deleted the config-specific-imports branch August 29, 2018 19:36

		@@ -0,0 +1,5 @@
		// Copyright (c) 2017, the Dart project authors. Please see the AUTHORS file


		import 'package:test/test.dart';

		// ignore: uri_does_not_exist

Adds support for config specific imports #1793

Adds support for config specific imports #1793

Conversation

jakemac53 commented Aug 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pulyaevskiy commented Aug 28, 2018

jakemac53 commented Aug 28, 2018

grouma commented Aug 28, 2018

natebosch commented Aug 28, 2018

jakemac53 commented Aug 28, 2018

vsmenon commented Aug 28, 2018

jakemac53 commented Aug 29, 2018

jakemac53 commented Aug 29, 2018