-
Notifications
You must be signed in to change notification settings - Fork 231
After install scripts (#1904) #1908
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
Changes from all commits
2251f91
8142abc
85dd8f9
72d83e7
f915d40
5ff12eb
89e0950
74a90c8
f3ebbba
da33bab
cd2df88
2091589
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,81 @@ | ||
// Copyright (c) 2015, the Dart project authors. Please see the AUTHORS file | ||
// for details. All rights reserved. Use of this source code is governed by a | ||
// BSD-style license that can be found in the LICENSE file. | ||
|
||
import 'dart:async'; | ||
import 'dart:convert'; | ||
import 'dart:io'; | ||
import 'package:path/path.dart' as p; | ||
|
||
/// Stores timestamps of the absolute paths to Dart scripts | ||
/// specified in packages' "after_install" fields, and the | ||
/// last times they were run. | ||
class AfterInstallCache { | ||
final Map<String, int> _cache; | ||
|
||
const AfterInstallCache._(this._cache); | ||
|
||
/// Returns the name of the file that would hold an "after_install" cache in the [rootDir]. | ||
static String resolveCacheFilePath(String rootDir) { | ||
return p.join(rootDir, "after_install_cache.json"); | ||
} | ||
|
||
/// Loads from a given [rootDir]. | ||
static Future<AfterInstallCache> load(String rootDir) async { | ||
var filename = resolveCacheFilePath(rootDir); | ||
var cacheFile = new File(filename); | ||
|
||
// If the file does not exist, return the default. | ||
if (!await cacheFile.exists()) return new AfterInstallCache._({}); | ||
|
||
var map = json.decode(await cacheFile.readAsString()); | ||
var isValidMap = map is Map && | ||
map.keys.every((k) => k is String) && | ||
map.values.every((k) => k is int); | ||
|
||
// If the file is formatted improperly, return the default. | ||
// | ||
// Whatever corrupted data was stored in the cache file, will | ||
// ultimately be overwritten. | ||
if (!isValidMap) return new AfterInstallCache._({}); | ||
|
||
// If everything went well, return the parsed cache. | ||
return new AfterInstallCache._(map.cast<String, int>()); | ||
} | ||
|
||
/// Determines if the script at [path] should be re-run. | ||
/// | ||
/// This is `true` if any of the following is true: | ||
/// * The cache contains no entry for the [path]. | ||
/// * The file at [path] was modified after the timestamp in the cache. | ||
Future<bool> isOutdated(String path) async { | ||
if (!_cache.containsKey(path)) return true; | ||
var stat = await FileStat.stat(path); | ||
return _cache[path] < stat.modified.millisecondsSinceEpoch; | ||
} | ||
|
||
/// Saves the contents of the cache to a file in the given [rootDir]. | ||
Future save(String rootDir) async { | ||
if (_cache.isNotEmpty) { | ||
var file = new File(resolveCacheFilePath(rootDir)); | ||
await file.create(recursive: true); | ||
await file.writeAsString(json.encode(_cache)); | ||
} | ||
} | ||
|
||
/// Updates the cached timestamp for [path]. | ||
void update(String path) => | ||
_cache[path] = new DateTime.now().millisecondsSinceEpoch; | ||
|
||
/// Returns a combined cache containing the contents of both `this` and [other]. | ||
/// | ||
/// Where there are conflicts, values in [other] are prioritized. | ||
/// | ||
/// Does not modify the caches of either `this` or [other]. | ||
AfterInstallCache merge(AfterInstallCache other) { | ||
return new AfterInstallCache._( | ||
<String, int>{}..addAll(_cache)..addAll(other._cache)); | ||
} | ||
|
||
Map<String, int> toMap() => new Map<String, int>.from(_cache); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,12 +4,14 @@ | |
|
||
import 'dart:async'; | ||
import 'dart:io'; | ||
import 'dart:isolate'; | ||
|
||
import 'package:collection/collection.dart'; | ||
import 'package:package_config/packages_file.dart' as packages_file; | ||
import 'package:path/path.dart' as p; | ||
import 'package:pub_semver/pub_semver.dart'; | ||
|
||
import 'after_install.dart'; | ||
import 'dart.dart' as dart; | ||
import 'exceptions.dart'; | ||
import 'http.dart' as http; | ||
|
@@ -246,6 +248,8 @@ class Entrypoint { | |
} else { | ||
_deleteExecutableSnapshots(changed: result.changedPackages); | ||
} | ||
|
||
await runAfterInstallScripts(result); | ||
} catch (error, stackTrace) { | ||
// Just log exceptions here. Since the method is just about acquiring | ||
// dependencies, it shouldn't fail unless that fails. | ||
|
@@ -642,4 +646,127 @@ class Entrypoint { | |
ensureDir(p.dirname(newPath)); | ||
renameDir(oldPath, newPath); | ||
} | ||
|
||
/// Execute any outstanding Dart scripts in `after_install`. | ||
Future runAfterInstallScripts(SolveResult result) async { | ||
// Count how many scripts we could potentially run. | ||
var allScripts = <String, List<String>>{}; | ||
|
||
for (var package in result.changedPackages) { | ||
var scripts = result.pubspecs[package].afterInstall; | ||
if (scripts.isNotEmpty) allScripts[package] = scripts; | ||
} | ||
|
||
// If no scripts need to be run, don't bother updating the cache. | ||
if (allScripts.isEmpty) return; | ||
|
||
// Figure out what the last time every script was run. | ||
// If a script has not changed since the last time it was run, | ||
// Don't run it. | ||
var systemScriptCache = await AfterInstallCache.load(cache.rootDir); | ||
var localScriptCache = p.equals(cache.rootDir, cachePath) | ||
? systemScriptCache | ||
: await AfterInstallCache.load(cachePath); | ||
var mergedCache = systemScriptCache.merge(localScriptCache); | ||
|
||
// Sort so that the root's scripts run last. | ||
var changedPackages = result.changedPackages.toList(); | ||
changedPackages.sort((a, b) => a == root.name ? 1 : a.compareTo(b)); | ||
|
||
// Run scripts in dependencies. | ||
for (var package in changedPackages) { | ||
var pkg = package == root.name | ||
? root | ||
: cache.load(result.packages | ||
.firstWhere((a) => a.name == package, orElse: () => null)); | ||
|
||
for (var script in pkg.pubspec.afterInstall) { | ||
var absolutePath = p.normalize(p.absolute(p.join(pkg.dir, script))); | ||
var scriptFile = new File(absolutePath); | ||
|
||
// Don't run the script against there are new changes. | ||
if (!await mergedCache.isOutdated(absolutePath)) continue; | ||
|
||
if (!await scriptFile.exists()) { | ||
if (pkg == root) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit, use block syntax if it can't fit on one line |
||
log.warning(log.yellow( | ||
'"after_install" script "$absolutePath" does not exist.')); | ||
} else { | ||
// Never trust any third-party scripts. Show a prompt before running scripts. | ||
// | ||
// The user can opt to print the contents of the script. | ||
YesNoPrintResponse response = YesNoPrintResponse.yes; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: omit the type on the left (just use |
||
|
||
if (pkg != root) { | ||
var message = | ||
'\npackage:$package wants to run the script "$absolutePath".'; | ||
|
||
do { | ||
response = await confirmOrPrint(message); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We would need to figure out how to handle this on CI systems like travis - or within editors like VsCode/Intellij There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a valid concern, and something I didn't consider at all. At least for Travis, in theory a command-line arg or env variable could be used to auto-deny (or maybe auto-allow) every script? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes - I think the default behavior would need to be to auto-deny, with a flag or environment variable to auto-allow. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we consider being able to whitelist scripts from certain packages? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I like that idea |
||
|
||
if (response == YesNoPrintResponse.no) { | ||
log.message('User declined to run script.'); | ||
continue; | ||
} | ||
|
||
if (response == YesNoPrintResponse.print) { | ||
log.message(log.gray('Contents of "$absolutePath":')); | ||
log.message(log.gray(await scriptFile.readAsString())); | ||
} | ||
} while (response == YesNoPrintResponse.print); | ||
} | ||
|
||
if (response != YesNoPrintResponse.yes) continue; | ||
|
||
// We need to change into the package's directory to run its scripts. | ||
var currentDir = Directory.current; | ||
Directory.current = pkg.dir; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We shouldn't need to do this (and its probably not what many scripts would want, they may want to look at the current package and would expect that to be the current directory). We definitely dont want to run inside the package cache like this would do, as touching anything in there is forbidden. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good, more than likely could be removed |
||
|
||
// Spawn an isolate for the script. | ||
var onError = new ReceivePort(); | ||
var onExit = new ReceivePort(); | ||
var onComplete = new Completer(); | ||
|
||
onExit.listen( | ||
(x) => onComplete.isCompleted ? null : onComplete.complete()); | ||
onError.listen((x) => onComplete.isCompleted | ||
? null | ||
: onComplete.completeError(x[0], x[1])); | ||
|
||
try { | ||
Isolate.spawnUri(p.toUri(absolutePath), [], null, | ||
onExit: onExit.sendPort, | ||
onError: onError.sendPort, | ||
automaticPackageResolution: true); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should use the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's probably the solution I was looking for, haha. Is there a way to specify a working directory for an isolate, though? In my head, the scripts would execute in the folder for the specific version that package, so that things like native extensions could be built to the correct paths. Otherwise, though, people publishing scripts could still just There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
We really don't want to give access to the actual package cache. That should only contain the exact sources as they exist on pub because it is shared between all projects on your machine. The output of the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, that's fair, and makes a lot of sense. One more question: what can end-users do to depend on sources built to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is actually a really good question haha - basically the build system knows how to surface those files via the We would have to come up with some other solution here for pub, possibly writing custom paths to the |
||
await onComplete.future; | ||
|
||
// Update the cache so this script does not run again redundantly. | ||
if (pkg == root) | ||
localScriptCache.update(absolutePath); | ||
else | ||
systemScriptCache.update(absolutePath); | ||
} catch (e, st) { | ||
log.error( | ||
log.red('An unhandled exception occurred in "$absolutePath".'), | ||
log.red(e), | ||
st); | ||
log.error(log | ||
.red('Below is the stack trace where the exception occurred:')); | ||
log.error(log.red(st.toString())); | ||
log.error(log.red( | ||
'\nThis is not an error in Pub; Report this error to the author(s) of package:$package instead.')); | ||
} finally { | ||
// Switch back! | ||
Directory.current = currentDir; | ||
onError.close(); | ||
onExit.close(); | ||
} | ||
} | ||
} | ||
} | ||
|
||
// Now that we've run all scripts, update the cache files. | ||
await systemScriptCache.save(cache.rootDir); | ||
await localScriptCache.save(cachePath); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that only checking the timestamp here is not a full solution. There could be arbitrary imports included which would also technically need to be checked which would be a lot more cumbersome.
I think we should re-think a little bit how we determine when to run "after_install" scripts.
For instance, one really awesome use case for this feature would be the ability to provide automatic upgrade scripts with a package. These would have to be tied to specific releases - and when upgrading from one release to another it should run all of the update scripts for all intermediate releases in order.