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

Pub Transformers running in parallel instead of in stages #13

Closed
DartBot opened this issue Jun 4, 2015 · 11 comments
Closed

Pub Transformers running in parallel instead of in stages #13

DartBot opened this issue Jun 4, 2015 · 11 comments
Labels
closed-as-intended Closed as the reported issue is expected behavior type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@DartBot
Copy link

DartBot commented Jun 4, 2015

<img src="https://avatars.githubusercontent.com/u/2130849?v=3" align="left" width="96" height="96"hspace="10"> Issue by kaendfinger
Originally opened as dart-lang/sdk#23323


What steps will reproduce the problem?

  1. Create a new package that uses 2 transformers
  2. Run transformers
  3. Notice that transformers run in parallel

What is the expected output? What do you see instead?

with the given transformer definitions:

transformers:

  • transformer_one
  • transformer_two

this should run the first transformer and then the second one.

What version of the product are you using?

Dart VM version: 1.11.0-edge.45391 (Thu Apr 23 17:40:55 2015) on "macos_x64"

On what operating system?

Mac OS X

What browser (if applicable)?

N/A

Please provide any additional information below.

According to https://www.dartlang.org/tools/pub/assets-and-transformers.html

To specify that transformers run in parallel, use [transformer_1, ..., transformer_n]. If order matters, put the transformers on separate lines.

For example, consider three transformers, specified as follows:

transformers:

  • [t1, t2]
  • t3
@DartBot
Copy link
Author

DartBot commented Jun 4, 2015

<img src="https://avatars.githubusercontent.com/u/5479?v=3" align="left" width="48" height="48"hspace="10"> Comment by sethladd


Added Area-Pub, Triaged labels.

@DartBot
Copy link
Author

DartBot commented Jun 4, 2015

<img src="https://avatars.githubusercontent.com/u/188?v=3" align="left" width="48" height="48"hspace="10"> Comment by nex3


What exactly makes you think they're running in parallel?


Added NeedsInfo label.

@DartBot DartBot added type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) closed-as-intended Closed as the reported issue is expected behavior labels Jun 4, 2015
@DartBot
Copy link
Author

DartBot commented Jun 4, 2015

<img src="https://avatars.githubusercontent.com/u/2130849?v=3" align="left" width="48" height="48"hspace="10"> Comment by kaendfinger


When I added print statements to my 2 transformers, around the work they do, it went like this:

transformer 1 started
transformer 2 started
transformer 1 finished
transformer 2 finished

@DartBot
Copy link
Author

DartBot commented Jun 4, 2015

<img src="https://avatars.githubusercontent.com/u/188?v=3" align="left" width="48" height="48"hspace="10"> Comment by nex3


What I should have said was, please provide code that demonstrates this issue.

@DartBot
Copy link
Author

DartBot commented Jun 4, 2015

<img src="https://avatars.githubusercontent.com/u/2130849?v=3" align="left" width="48" height="48"hspace="10"> Comment by kaendfinger


Use the following pubspec:

name: pub_transformer_bug
dependencies:
  libffi6_extension: ">=0.0.3 <0.0.4"
  unsafe_extension: ">=0.0.16 <0.0.17"
transformers:

  • unsafe_extension
  • libffi6_extension

Modify .pub-cache/hosted/pub.dartlang.org/libffi6_extension-0.0.3/lib/transformer.dart and .pub-cache/hosted/pub.dartlang.org/unsafe_extension-0.0.16/lib/transformer.dart and add print statements around the following code in each file:

    try {
      var path = _resolvePackagePath(filepath);
      FileUtils.chdir(path);
      var installer = new Installer();
      await installer.install([]);
    } finally {
      FileUtils.chdir(_workingDirectory);
    }

run pub get

@DartBot
Copy link
Author

DartBot commented Jun 4, 2015

<img src="https://avatars.githubusercontent.com/u/188?v=3" align="left" width="48" height="48"hspace="10"> Comment by nex3


Changing the working directory is never going to work out well for you in transformers. It's 100% intended for transformers to run in parallel on multiple files, so you're going to have serious race conditions any time you have multiple transformers modifying the same global state, including the working directory.

@DartBot
Copy link
Author

DartBot commented Jun 4, 2015

<img src="https://avatars.githubusercontent.com/u/2130849?v=3" align="left" width="48" height="48"hspace="10"> Comment by kaendfinger


Ahh I didn't even pay attention to the fact that it's doing that. So that would be the issue I guess then, correct? If so, go ahead in close this. The reason why we change directories for these transformers is really just a hack (acts like an install hook for us).

Thanks for checking into it though!

@DartBot
Copy link
Author

DartBot commented Jun 4, 2015

<img src="https://avatars.githubusercontent.com/u/188?v=3" align="left" width="48" height="48"hspace="10"> Comment by nex3


Added AsDesigned label.

@DartBot
Copy link
Author

DartBot commented Jun 4, 2015

<img src="https://avatars.githubusercontent.com/u/2311549?v=3" align="left" width="48" height="48"hspace="10"> Comment by mezoni


It's 100% intended for transformers to run in parallel on multiple files, so you're going to have serious race conditions any time you have multiple transformers modifying the same global state, including the working directory.

Need special kind of the transformers which can be sure that their instance run exclusively.

Eg.

abstract class PreinstallTransformer implements Transformer {
  Future apply(InstallationTransform transform);
}

abstract class PostinstallTransformer implements Transformer {
  Future apply(InstallationTransform transform);
}

Where "InstallationTransform" should provide the path to the package in pub-cache.

@DartBot
Copy link
Author

DartBot commented Jun 4, 2015

<img src="https://avatars.githubusercontent.com/u/2311549?v=3" align="left" width="48" height="48"hspace="10"> Comment by mezoni


Better way:

abstract class GlobalTransformer implements Transformer {
  String get acceptedPhases;

  Future apply(GlobalTransform transform);
}

Eg.

class MyInstallTransformer extends GlobalTransformer {
  String get acceptedPhases = "preinstall";

  Future apply(GlobalTransform transform) {
    switch(transform.phase) {
      case "preinstall":
        preinstall(transform.packageRoot);
        break;
    }
  }
}

@DartBot
Copy link
Author

DartBot commented Jun 4, 2015

<img src="https://avatars.githubusercontent.com/u/2311549?v=3" align="left" width="48" height="48"hspace="10"> Comment by mezoni


I am more than sure that the developers never did anything for the minority (read: for the community). Only for the majority.

And as all we know that the majority is a developers themselves, and the minority is an all rest of the people in the world.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-as-intended Closed as the reported issue is expected behavior type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Development

No branches or pull requests

1 participant