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

Support creating output directories that only contain symlinks #1647

Merged
merged 26 commits into from
Jul 24, 2018

Conversation

jakemac53
Copy link
Contributor

Closes #1242

Added a new --symlink option, which is defaulted to true for test but no other commands.

This is unfortunately a breaking change in build_runner and build_runner_core as it required adding a new interface which must be implemented by any RunnerAssetReader, which is an exposed class from both of these packages.

@jakemac53 jakemac53 requested a review from natebosch July 11, 2018 21:12
@googlebot googlebot added the cla: yes Google is happy with the PR contributors label Jul 11, 2018
@jakemac53
Copy link
Contributor Author

In terms of performance, when testing with the pub package and the new vm compilation pipeline it takes creating the merged directory from >40 seconds to <1 second. Note that the output directory is unnecessarily large since we don't clean up the modular dill files in that case, or the original dart files, but its still a good proxy I think for how this can affect large packages.

@jakemac53
Copy link
Contributor Author

Tried running this with angulars _tests and looks like web tests don't work 🤦‍♂️ . Investigating now... but will require a fix in package:test it looks like.


### Breaking Changes

- The `RunnerAssetReader` interface now requires that you implement the new
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe call out that most users don't have any reason to implement this interface and are not likely impacted.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Member

@natebosch natebosch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed offline - we should hold off on committing this until we've gotten another publish of build_runner that supports Dart 2.

@jakemac53
Copy link
Contributor Author

cc @natebosch ok I updated this with the changes we talked about.

There is now a finalizeBuild method on BuildEnvironment, and we use that to create the merged output dir. Only the IOEnvironment knows about the new asset reader interface.

@jakemac53
Copy link
Contributor Author

Looks like symlinks don't work as expected on windows even though there is supposedly support for linking individual files according to windows docs, the VM always ends up creating a directory symlink.

@jakemac53
Copy link
Contributor Author

Filed dart-lang/sdk#33966

@@ -17,6 +17,8 @@ import '../package_graph/package_graph.dart';
import 'build_environment.dart';
import 'create_merged_dir.dart';

final _logger = new Logger('IOEnvironment');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to include this at

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@jakemac53 jakemac53 merged commit 6066471 into master Jul 24, 2018
@jakemac53 jakemac53 deleted the better-create-output-dir branch July 24, 2018 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Google is happy with the PR contributors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants