Drop build and serve #1871

natebosch · 2018-04-14T01:14:02Z

Drop lib/src/{barback,dartdevc,asset} and relevant tests
Add lib/src/asset/id.dart since non-building code uses this type
from barback.
Clean up analyzer errors, droping any code paths that relate to
dealing with Transformers, for example the run command's ability to
run with transformed code, or any parsing and plumbing for transformer
config.
Clean up any tests that were checking behavior of transformers or the
build or serve command.
Remove any dependencies for which I could no longer find an import.

- Drop `lib/src/{barback,datdevc,asset}` and relevant tests - Add `lib/src/asset/id.dart` since non-building code uses this type from barback. - Clean up analyzer errors, droping any code paths that relate to dealing with Transformers, for example the `run` command's ability to run with transformed code, or any parsing and plumbing for transformer config. - Clean up any tests that were checking behavior of transformers or the build or serve command. - Remove any dependencies for which I could no longer find an import.

natebosch · 2018-04-14T01:15:19Z

Note that a pub run on a package which does expected to be transformed will now likely fail in mysterious ways since it will be run directly without transformers. I tried to see if we could cheaply keep around just enough of the code paths to support a warning for this case but it didn't seem like it would be worth doing.

natebosch · 2018-04-14T01:17:40Z

The build or serve commands now exit(1) after printing in red to stderr:

Dart 2 has a new build system. Learn how to migrate from pub build and
pub serve: https://webdev.dartlang.org/dart-2

- Hide the build and serve commands - Remove mode option

kevmoo · 2018-04-14T02:34:59Z

one failing test – and one unformatted file. Nice...

natebosch · 2018-04-14T04:56:59Z

@nex3 - can you help me understand how this test works?
https://github.com/dart-lang/pub/blob/master/test/global/run/uses_old_lockfile_test.dart

I see that it's setting up a fake package for foo. How does the import to bar work here? I don't see where bar.main() is getting set up with some behavior. The failure Could not find bin/script.dart in package foo. makes sense to me, how did this work before?

nex3 · 2018-04-16T21:03:09Z

Will do a full review in a bit, but:

Note that a pub run on a package which does expected to be transformed will now likely fail in mysterious ways since it will be run directly without transformers. I tried to see if we could cheaply keep around just enough of the code paths to support a warning for this case but it didn't seem like it would be worth doing.

I think this is fine. I think the vast majority of users of transformers in practice used them for building web apps.

@nex3 - can you help me understand how this test works?
https://github.com/dart-lang/pub/blob/master/test/global/run/uses_old_lockfile_test.dart

I see that it's setting up a fake package for foo. How does the import to bar work here? I don't see where bar.main() is getting set up with some behavior. The failure Could not find bin/script.dart in package foo. makes sense to me, how did this work before?

This tries to simulate what pub global activate would have created with older pub versions—the foo package isn't fake, it's just added to the cache manually via pub cache add on line 26.

The bar import worked because pub would load up an asset environment every time it ran an executable. The right behavior now is probably to detect an executable without a snapshot and try to compile that snapshot.

natebosch · 2018-04-16T21:08:00Z

The bar import worked because pub would load up an asset environment every time it ran an executable.

If I'm understanding the test correctly it seems to expect that the asset environment would have successfully provided that file, allow the pub run to execute the script, and somehow make bar.main() return "bar 1.0.0"

Or am I misunderstanding this?

natebosch · 2018-04-16T21:20:00Z

Ah, Jake pointed me to where the magic file is created

natebosch · 2018-04-16T21:51:55Z

The immediate problem is that without the Barback asset environment we only know how to find a package if it has a .packages file.

I think the "1.6-style lockfile" for global packages might only work through the asset environment.

Before I spend time on a fix - is this something we feel we need to continue to support?

This is not worth supporting

nex3 · 2018-04-16T23:18:59Z

The immediate problem is that without the Barback asset environment we only know how to find a package if it has a .packages file.

I think the "1.6-style lockfile" for global packages might only work through the asset environment.

Right. Before we started creating snapshots (which was well after 1.6), all pub global run invocations used the asset environment, so they weren't set up to work in any other way.

Before I spend time on a fix - is this something we feel we need to continue to support?

If it's a huge amount of effort you could drop it, but I think all you should need to do is something like

if (!fileExists(snapshotPath)) await _precompileExecutables(package);

There's a lot of psychological value for users in guaranteeing that the package manager will never choke on data it itself created, even in edge cases like this.

This reverts commit 5baf955.

natebosch · 2018-04-16T23:39:37Z

If it's a huge amount of effort you could drop it, but I think all you should need to do is something like
if (!fileExists(snapshotPath)) await _precompileExecutables(package);

_precompileExecutables would end up going through snapshot which requires a .packages or packages/ as well (I don't know what "a package spec is inferred from the executable's location" means, but I suspect it won't happen since we're calling dart which doesn't understand package_name.lock files AFAIK).

I think in order to support this we'd need to rewrite some of the AssetEnvironment stuff without barback.

nex3

Real excited to see this stuff get deleted!

nex3 · 2018-04-16T23:22:51Z

lib/src/asset/id.dart

+// BSD-style license that can be found in the LICENSE file.
+
+/// Like the AssetId class from barback but without the barback dependency.
+class AssetId {


Is it possible to get rid of this entirely? Looking through the remaining uses, it seems like almost all of them exist in contexts where the package is determined by context, so you could just use the executable name.

I tried that at first and hit at least one case where we were storing a collection of AssetIds and lost context of their associated packages.

I can take another look, but I'd prefer to do that in a new PR.

I overestimated the scope. I replaced id with a packageName and pathInPackage argument in the snapshot method, but everything else was, as you pointed out, already in a context where we had the package name.

nex3 · 2018-04-16T23:24:23Z

lib/src/cached_package.dart

-/// cached, this returns the cached information. It also wraps the package's
-/// pubspec to report no transformers, since the transformations have all been
-/// applied already.
+/// cached, this returns the cached information.
 class CachedPackage extends Package {


This shouldn't be necessary anymore—there's nothing for pub to precompile in the lib directory.

nex3 · 2018-04-16T23:25:20Z

lib/src/command/barback.dart

-    return result;
+    log.error(log.red("Dart 2 has a new build system. Learn how to migrate "
+        "from ${log.bold('pub build')} and\n"
+        "${log.bold('pub serve')}: https://webdev.dartlang.org/dart-2\n"));


Nit: use fail() instead of log.error() and exit().

nex3 · 2018-04-16T23:26:50Z

lib/src/command/global_run.dart

  GlobalRunCommand() {
    argParser.addFlag("checked",
        abbr: "c", help: "Enable runtime type checks and assertions.");
-    argParser.addOption("mode",
-        defaultsTo: "release", help: 'Mode to run transformers in.');


If we're going to give build and serve helpful error messages, we might as well do so for --mode as well (also below).

nex3 · 2018-04-16T23:28:48Z

lib/src/dart.dart

@@ -8,116 +8,13 @@ import 'dart:io';
 import 'dart:isolate';


It looks like isPart() in this file is no longer used.

Removed along with runInIsolate and some (newly?) unused methods from utils.dart

nex3 · 2018-04-16T23:39:22Z

lib/src/entrypoint.dart

    });
  }

  //// Precompiles [executables] to snapshots from the filesystem.
-  Future _precompileExecutablesWithoutBarback(
-      Map<String, List<AssetId>> executables) {
+  Future _precompileExecutables(Map<String, List<AssetId>> executables) {


For example, the package names in the executables map are totally redundant; this could just be Map<String, List<String>>. The only reason we represented executables as AssetIds before was for convenience when passing them to Barback.

yeah, I started down that path and hit one place where it wasn't redundant and restructuring that felt like it was snowballing.

nex3 · 2018-04-16T23:42:56Z

lib/src/pubspec.dart

@@ -245,82 +243,6 @@ class Pubspec {

  Map<String, Feature> _features;

-  /// The configurations of the transformers to use for this package.
-  List<Set<TransformerConfig>> get transformers {


It would be good to add validators that produce warnings when trying to publish packages that include a transformers field or a web.compiler field.

Can we do that in a followup?

nex3 · 2018-04-16T23:43:54Z

lib/src/solver/version_solver.dart

-        new Term(
-            _systemCache.sources.hosted.refFor(name).withConstraint(constraint),
-            false)
-      ], IncompatibilityCause.pubDependency));


Please also remove IncompatibilityCause.pubDependency and its other referents.

nex3 · 2018-04-16T23:46:39Z

test/global/run/uses_old_lockfile_test.dart

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


We should still behave nicely with old-style packages—ideally recompile them, if not that then create a .packages file, or at the very least produce a useful error message telling users how to fix the problem.

nex3 · 2018-04-16T23:47:21Z

test/package_server.dart

-/// pub.
-Future serveBarback() {
-  return servePackages((builder) {
-    builder.serveRealPackage('barback');


I think you can delete serveRealPackage() as well.

nex3 · 2018-04-16T23:49:46Z

_precompileExecutables would end up going through snapshot which requires a .packages or packages/ as well (I don't know what "a package spec is inferred from the executable's location" means, but I suspect it won't happen since we're calling dart which doesn't understand package_name.lock files AFAIK).

I suppose that's true, but you can generate a new .packages file really easily given the lockfile.

natebosch · 2018-04-16T23:55:52Z

Ok, I've got a change that passes the test. bcc70cd

It requires passing the system cache through, we could maybe do this check earlier though to avoid it introducing a new import in executable.dart

nex3

Ok, I've got a change that passes the test. bcc70cd

It requires passing the system cache through, we could maybe do this check earlier though to avoid it introducing a new import in executable.dart

LGTM. I don't think adding a new import is a big cost, but I realized you'd still have to check to make sure that it's immutable and fall back to running from source if it's not, so just writing the .packages file is probably fine. Make sure to re-add the test, though!

nex3 · 2018-04-17T00:08:30Z

lib/src/executable.dart

+  if (!fileExists(entrypoint.packagesFile)) {
+    if (!isGlobal) return null;
+    await writeTextFile(
+        entrypoint.packagesFile, entrypoint.lockFile.packagesFile(cache));


Add a comment indicating why a global entrypoint without a .packages file would exist.

nex3 · 2018-04-17T01:10:38Z

Give me a poke when you want me to take another look.

natebosch · 2018-04-17T01:40:42Z

I think I have addressed all feedback ~~except for:~~

~~Validation that there is no transformers or web compiler config on publish~~
~~Warning when using the --mode argument to run~~

~~If you're OK with adding validation in a follow up~~ this is ready for another review.

jakemac53 · 2018-04-17T14:05:34Z

Fwiw - I am going to forgo doing an actual review here since @nex3 is more familiar with the codebase anyways and its quite large.

I will note that when it comes to supporting the old lockfiles for pub run the chance of those globally activated packages being able to run with old package versions on the Dart 2 sdk is essentially zero anyways, so breaking compatibility with them is perfectly fine with me. We should just detect the situation and ask people to re-activate the package.

nex3 · 2018-04-17T20:27:59Z

lib/src/dart.dart

 Future snapshot(Uri executableUrl, String snapshotPath,
-    {Uri packagesFile, AssetId id}) async {
-  var name = log.bold(id == null
+    {Uri packagesFile, String packageName, String pathInPackage}) async {


Consider replacing the packageName and pathInPackage parameters with an name parameter and make the caller responsible for deciding what that name could look like. Then the first line could just be name ??= executableUrl.toString().

nex3 · 2018-04-17T20:28:28Z

lib/src/executable.dart

+  // helpful for the subprocess to be able to spawn Dart with
+  // Platform.executableArguments and have that work regardless of the working
+  // directory.
+  Uri packageConfig = p.toUri(p.absolute(entrypoint.packagesFile));


Nit: don't type-annotate local variables.

nex3 · 2018-04-17T20:32:44Z

lib/src/validator/deprecated_fields.dart

+  DeprecatedFieldsValidator(Entrypoint entrypoint) : super(entrypoint);
+
+  Future validate() {
+    return new Future.sync(() {


Nit: I'd just make the method async.

nex3 · 2018-04-17T20:33:08Z

lib/src/validator/deprecated_fields.dart

+    return new Future.sync(() {
+      if (entrypoint.root.pubspec.fields.containsKey('transformers')) {
+        warnings.add('Your pubpsec.yaml includes a "transformers" section which'
+            ' is no longer used and may be removed');


Nit: warnings should end with a period.

nex3 · 2018-04-17T20:34:25Z

test/validator/deprecated_fields_test.dart

+
+main() {
+  setUp(d.validPackage.create);
+


It would be good to add a test that this doesn't warn for a pubspec that doesn't have either deprecated field.

nex3 · 2018-04-17T20:50:47Z

denniskaselow · 2018-04-24T06:01:08Z

I don't want to ruin the party, but running pub get/upgrade from master now hangs on windows after commit 5ca86eb.

It does
IO : Spawning "cmd /c dart --packages=.packages --snapshot=.\.dart_tool/pub\bin\test\bin\test.dart.snapshot file:///C:/Users/dennis/AppData/Roaming/Pub/Cache/hosted/pub.dartlang.org/test-0.12.34/bin%5Ctest.dart" in F:\Programming\workspaces\dart\dartemis_builder\.

instead of

IO : Spawning "cmd /c dart --packages=.packages --snapshot=.\.dart_tool/pub\bin\test\bin\test.dart.snapshot file:///C:/Users/dennis/AppData/Roaming/Pub/Cache/hosted/pub.dartlang.org/test-0.12.34/bin/test.dart" in F:\Programming\workspaces\dart\dartemis_builder\.

(notice the encoded \ (%5C) instead of a /.

Stumbled over it, when I tested the fix for #1756.

natebosch · 2018-04-24T06:29:01Z

This is not synced in the SDK so it is not published. What configuration are you using such that you are running this version of pub?

denniskaselow · 2018-04-24T06:30:38Z

I cloned the repository to test that other bugfix and used dart bin/pub.dart to run it.

I dove in a bit deeper, and the problem for the %5C problem is 5ca86eb. It changes what precompiling executables does.
If I go back to 711f515 dart bin/pub.dart runs just fine.

kevmoo · 2018-04-24T22:07:54Z

@denniskaselow – could we open a separate issue for this? in the pub repo...

Please be clear on the commit SHA you're hitting the issue at...

natebosch requested review from nex3 and jakemac53 April 14, 2018 01:14

googlebot added the cla: yes label Apr 14, 2018

Clean up more references to Transformer

9da867b

- Hide the build and serve commands - Remove mode option

dartfmt

0e8bca0

Drop uses_old_lockfile test

5baf955

This is not worth supporting

Revert "Drop uses_old_lockfile test"

894ac32

This reverts commit 5baf955.

nex3 suggested changes Apr 16, 2018

View reviewed changes

Support old lockfiles by writing a new .packages file

bcc70cd

natebosch added 3 commits April 16, 2018 17:00

Drop CachedPackage

ebf1009

Switch to fail

e78b689

Removed unused dart.dart utilities

0eed5a5

nex3 reviewed Apr 17, 2018

View reviewed changes

natebosch added 4 commits April 16, 2018 17:11

Unused utils

fce71a2

Always use a full path for executable

e7587c0

Inline _precompileExecutablesWithoutBarback

afd9a9e

Remove IncompatibilityCause.pubDependency

9237ae8

natebosch added 4 commits April 16, 2018 17:23

Remove serveRealPackage test util

3a9ed19

Add comment about old global lock files

0efccc1

Unused imports

e7ffbb7

Drop AssetId class

a33e570

dartfmt

db2cff4

natebosch added 3 commits April 17, 2018 12:11

Add hint to avoid --mode flag

8a8e090

Add warning when publishing with a transformer section

f634d9d

dartfmt

2c919b7

nex3 reviewed Apr 17, 2018

View reviewed changes

nex3 mentioned this pull request Apr 17, 2018

Cannot use pub with --preview-dart-2 dart-lang/sdk#32188

Closed

natebosch added 2 commits April 17, 2018 13:40

Replace packageName and pathInPackage args with 'name'

0c68a02

Review nits and valid pubspec test

e5f842b

nex3 approved these changes Apr 17, 2018

View reviewed changes

natebosch added 2 commits April 17, 2018 14:01

Unused import

f412114

package.name not package

729f263

natebosch merged commit 5ca86eb into master Apr 17, 2018

natebosch deleted the kill-build branch April 17, 2018 21:28

natebosch mentioned this pull request Apr 24, 2018

Precompiling executables uses wrong paths on windows #1878

Closed

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


		main() {
		setUp(d.validPackage.create);

Drop build and serve #1871

Drop build and serve #1871

Conversation

natebosch commented Apr 14, 2018 • edited Loading

natebosch commented Apr 14, 2018

natebosch commented Apr 14, 2018

kevmoo commented Apr 14, 2018

natebosch commented Apr 14, 2018

nex3 commented Apr 16, 2018

natebosch commented Apr 16, 2018

natebosch commented Apr 16, 2018 • edited Loading

natebosch commented Apr 16, 2018

nex3 commented Apr 16, 2018

natebosch commented Apr 16, 2018

nex3 left a 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

natebosch Apr 17, 2018 • edited Loading

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

nex3 commented Apr 16, 2018

natebosch commented Apr 16, 2018

nex3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nex3 commented Apr 17, 2018

natebosch commented Apr 17, 2018 • edited Loading

jakemac53 commented Apr 17, 2018 • edited Loading

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

nex3 commented Apr 17, 2018 • edited Loading

denniskaselow commented Apr 24, 2018 • edited Loading

natebosch commented Apr 24, 2018

denniskaselow commented Apr 24, 2018 • edited Loading

kevmoo commented Apr 24, 2018

natebosch commented Apr 14, 2018 •

edited

Loading

natebosch commented Apr 16, 2018 •

edited

Loading

natebosch Apr 17, 2018 •

edited

Loading

natebosch commented Apr 17, 2018 •

edited

Loading

jakemac53 commented Apr 17, 2018 •

edited

Loading

nex3 commented Apr 17, 2018 •

edited

Loading

denniskaselow commented Apr 24, 2018 •

edited

Loading

denniskaselow commented Apr 24, 2018 •

edited

Loading