After install scripts (#1904) #1908

thosakwe · 2018-05-23T21:31:47Z

This is a working implementation of the changes proposed in #1904.

I've added tests for parsing the necessary value (an after_install field in the pubspec), but I'm not so sure of how to test the functionality, other than physically running it myself.

To clarify, the behavior verifiably works when run by a human, but I'm not so sure how to run it via computer.

One other thing I'd like to propose is an additional check upon running pub lish that throws an error if a script specified the pubspec does not actually exist on disk (to prevent breaks on end-user machines).

I'm willing to fix anything that needs to be fixed to land this PR. I really feel like something so simple as support for post-install scripts could go a long way to growing the Dart ecosystem.

Fix Dart 2 runtime errors (#1902)

…_install_scripts

nex3 · 2018-05-24T15:44:21Z

I won't have time to review this any time soon.

thosakwe · 2018-05-31T03:23:44Z

No prob... Anything I can do in the meantime to make the future review easier?

kevmoo

I'm excited to see interest here, but we'd have to do a substantial security review before we could add such a feature. I'll chat more with @nex3 when we're both in the office...

stepancar · 2018-06-14T10:47:14Z

@kevmoo any progress on this PR?

kevmoo · 2018-06-14T18:55:10Z

Still waiting on @nex3 to get back in the Office.

Again, this is a tough one. It's not about the code quality or understanding the use case – it's about security concerns.

thosakwe · 2018-06-14T19:06:34Z

Yeah, I can certainly understand that. If something like a post install hook could be used to do some sneaky business on users' computers, that certainly wouldn't be good.

I included an option for users to preview the source of all scripts that a package is requesting to run, so that could at least be a start. Also, the fact that after_install can only execute Dart scripts could potentially close at least a few holes.

nex3 · 2018-06-14T21:01:00Z

I don't have the time budgeted to review this, nor am I likely to any time soon.

jakemac53

Did a quick review just to get some conversation started here. I think this is great to get conversation started, even if we can't actually land it until Dart 2 settles down a little bit.

jakemac53 · 2018-06-15T19:37:32Z

lib/src/after_install.dart

+  /// 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 {


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.

jakemac53 · 2018-06-15T20:16:59Z

lib/src/entrypoint.dart

+        if (!await mergedCache.isOutdated(absolutePath)) continue;
+
+        if (!await scriptFile.exists()) {
+          if (pkg == root)


nit, use block syntax if it can't fit on one line

jakemac53 · 2018-06-15T20:17:42Z

lib/src/entrypoint.dart

+          // 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;


nit: omit the type on the left (just use var)

jakemac53 · 2018-06-15T20:19:15Z

lib/src/entrypoint.dart

+                '\npackage:$package wants to run the script "$absolutePath".';
+
+            do {
+              response = await confirmOrPrint(message);


We would need to figure out how to handle this on CI systems like travis - or within editors like VsCode/Intellij

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?

Yes - I think the default behavior would need to be to auto-deny, with a flag or environment variable to auto-allow.

should we consider being able to whitelist scripts from certain packages?

should we consider being able to whitelist scripts from certain packages?

I like that idea

jakemac53 · 2018-06-15T20:20:24Z

lib/src/entrypoint.dart

+
+          // We need to change into the package's directory to run its scripts.
+          var currentDir = Directory.current;
+          Directory.current = pkg.dir;


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.

Sounds good, more than likely could be removed

jakemac53 · 2018-06-15T20:30:20Z

lib/src/entrypoint.dart

+            Isolate.spawnUri(p.toUri(absolutePath), [], null,
+                onExit: onExit.sendPort,
+                onError: onError.sendPort,
+                automaticPackageResolution: true);


We should use the packageConfig argument and pass in the config from the current isolate, instead of automaticPackageResolution argument, similar to pub run behavior. This likely gets around the issue that was causing you to change directories to the other package?

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 Isolate.resolvePackageUri, probably

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.

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 after_install script could be different depending on the version solve of any given project. Therefore each individual project should have to re-run any after_install scripts and should generally write the results to the .dart_tool/<package-name> directory (by convention).

Okay, that's fair, and makes a lot of sense.

One more question: what can end-users do to depend on sources built to .dart_tool? By passing a custom package config? I'm not 100% sure how build does it.

That is actually a really good question haha - basically the build system knows how to surface those files via the serve command and within the build and treat it as an overlay file system. That isn't a general solution for all of .dart_tool though, its very specific to the .dart_tool/build/generated directory and the exact files that it knows it wrote there.

We would have to come up with some other solution here for pub, possibly writing custom paths to the .packages file that point to .dart_tool and copying the original package + generated sources there.

thosakwe · 2018-06-29T05:10:59Z

I've put more thought into this, and in the long run, I'm actually not so sure that after_install scripts will really be worth the potential security risks they introduce to Pub.

The main use case I had in mind for this functionality was distribution of native extensions; however, I've been able to get an early version of said functionality use existing infrastructure in build_native:
https://github.com/thosakwe/build_native

So, the merits of after_install scripts might actually be... Nothing?

edit: today, yet another NPM package with thousands of dependents was breached. It might not be possible to securely implement this at all.

mit-mit · 2019-02-20T13:34:35Z

Closing as it's my understanting we have decided not to move ahead with this.

thosakwe · 2019-10-07T13:50:32Z

If something like this is ever added to Pub, it occurred to me that instead of packages you depend having permission to run scripts, it would make more sense if your package could run hooks on get and upgrade, etc.

For example, you could create a .dart_tool/pub/hooks/post-get:

#!/usr/bin/env bash

# Build dependencies
pub run build_runner build --delete-conflicting-outputs

# Run some tool that compiles FFI libraries
pub run some_tool compile

That being said, I'm not sure if this would be useful to the majority of people using Pub, and it's already easy enough to just create a script that invokes pub get, rather than the other way around.

thosakwe added 12 commits May 23, 2018 17:15

Added afterInstall field/parser to lib/src/pubspec.dart

2251f91

Added tests for *parsing* after_install in test/pubspec_test.dart

8142abc

Create AfterInstallCache class

85dd8f9

Create yes/no/print prompt

72d83e7

Prompt before running, handle errors

f915d40

Auto-resolve packages

5ff12eb

Handle names without corresponding scripts

89e0950

Updated warning on nonexistent script

74a90c8

Merge pull request #1 from dart-lang/master

f3ebbba

Fix Dart 2 runtime errors (#1902)

Merge branch 'master' of https://github.com/thosakwe/pub-1 into after…

da33bab

…_install_scripts

Add header

cd2df88

Remove unnecessary import I added

2091589

googlebot added the cla: yes label May 23, 2018

natebosch requested a review from nex3 May 23, 2018 21:50

nex3 removed their request for review May 24, 2018 15:44

kevmoo requested changes May 31, 2018

View reviewed changes

jakemac53 reviewed Jun 15, 2018

View reviewed changes

mit-mit closed this Feb 20, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

After install scripts (#1904) #1908

After install scripts (#1904) #1908

thosakwe commented May 23, 2018

nex3 commented May 24, 2018

thosakwe commented May 31, 2018

kevmoo left a comment

stepancar commented Jun 14, 2018

kevmoo commented Jun 14, 2018

thosakwe commented Jun 14, 2018

nex3 commented Jun 14, 2018

jakemac53 left a comment

jakemac53 Jun 15, 2018

jakemac53 Jun 15, 2018

jakemac53 Jun 15, 2018

jakemac53 Jun 15, 2018

thosakwe Jun 15, 2018

jakemac53 Jun 15, 2018

natebosch Jun 19, 2018

jakemac53 Jun 19, 2018

jakemac53 Jun 15, 2018

thosakwe Jun 15, 2018

jakemac53 Jun 15, 2018

thosakwe Jun 15, 2018

jakemac53 Jun 15, 2018

thosakwe Jun 15, 2018

jakemac53 Jun 15, 2018

thosakwe commented Jun 29, 2018 •

edited

Loading

mit-mit commented Feb 20, 2019

thosakwe commented Oct 7, 2019

After install scripts (#1904) #1908

After install scripts (#1904) #1908

Conversation

thosakwe commented May 23, 2018

nex3 commented May 24, 2018

thosakwe commented May 31, 2018

kevmoo left a comment

Choose a reason for hiding this comment

stepancar commented Jun 14, 2018

kevmoo commented Jun 14, 2018

thosakwe commented Jun 14, 2018

nex3 commented Jun 14, 2018

jakemac53 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

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

thosakwe commented Jun 29, 2018 • edited Loading

mit-mit commented Feb 20, 2019

thosakwe commented Oct 7, 2019

thosakwe commented Jun 29, 2018 •

edited

Loading