Removed RestoreCommand and made assemblies load from packages/root folder by khellang · Pull Request #302 · scriptcs/scriptcs

khellang · 2013-05-26T19:27:57Z

Fixes #1

Please provide feedback on the current logic, it might not be 100%...

glennblock · 2013-05-27T04:45:15Z

One thing I worry about with this change is how it will impact performance. We don't want to have to hit nuget apis to find the reference paths every time we load. The first time it's ok.

How about if we create a file in the bin which contains all the paths you discovered for binaries? Then if the file exists you just use the paths within, and if it doesn't, you create it.

glennblock · 2013-05-27T04:51:04Z

src/ScriptCs/Command/ScriptCommand.cs

This feels like more of a service then a base command. Instead of using a base, what if you just have an IAssemblyPaths service which is injected into whoever needs it?

khellang · 2013-05-27T07:25:32Z

How about if we create a file in the bin which contains all the paths you discovered for binaries

I agree, we could even do this on installation?

glennblock · 2013-05-27T09:29:29Z

Yes, but remember if there's no bin today we run install for you. Similarly
I'd say if we find no path manifest we create it.

On Mon, May 27, 2013 at 12:25 AM, Kristian Hellang <[email protected]

wrote:

How about if we create a file in the bin which contains all the paths you
discovered for binaries

I agree, we could even do this on installation?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/302#issuecomment-18485792
.

khellang · 2013-05-27T10:04:36Z

We run installation automatically if there's a packages.config, but no packages folder.
An attempt to clarify the desired process:

Automatic installation:

Check for packages.config
Check for packages folder
If no packages folder exists, but a packages.config is present; install

Install:

Install packages in packages.config
Add paths to installed package binaries to the manifest in bin

Run:

Reference all "loose" binaries from the bin folder
If manifest exists, add all package binary paths from that

Have I forgotten anything?

glennblock · 2013-05-27T12:16:45Z

Looks good.

-----Original Message-----
From: "Kristian Hellang" [email protected]
Sent: ‎5/‎27/‎2013 3:04 AM
To: "scriptcs/scriptcs" [email protected]
Cc: "Glenn Block" [email protected]
Subject: Re: [scriptcs] Removed RestoreCommand and made assemblies load frompackages/root folder (#302)

We run installation automatically if there's a packages.config, but no packages folder.
An attempt to clarify the desired process:
Automatic installation:
Check for packages.config
Check for packages folder
If no packages folder exists, but a packages.config is present; install
Install:
Install packages in packages.config
Add paths to installed package binaries to the manifest in bin
Run:
Reference all "loose" binaries from the bin folder
If manifest exists, add all package binary paths from that
Have I forgotten anything?
—
Reply to this email directly or view it on GitHub.

khellang · 2013-05-27T12:49:25Z

How do you want the manifest? JSON, XML, CSV, separated lines, whatever?

jrusbatch · 2013-05-27T13:40:40Z

I think it would be a good idea to format this file in a way that makes it extensible. That way we can use the same file to store additional state if it becomes necessary in the future.

Users shouldn't need to view or edit the contents of the manifest file, so we don't necessarily need to limit ourselves to plain text formats. But for simplicity, 👍 JSON.

khellang · 2013-05-27T13:48:19Z

Alright! I'll just stick with ServiceStack.Text for the de-/serialization work, then 😄

khellang · 2013-05-27T13:51:50Z

@glennblock Regarding the IAssemblyPaths service you mentioned; should we combine IAssemblyNames with the GetAssemblyPaths and IsManagedAssembly methods into a single IAssemblyResolver service?

…lyResolver

glennblock · 2013-05-27T15:56:22Z

JSON please!

On Mon, May 27, 2013 at 6:51 AM, Kristian Hellang
[email protected]:

@glennblock https://github.com/glennblock Regarding the IAssemblyPathsservice you mentioned; should we combine
IAssemblyNames and the GetAssemblyPaths method into a single
IAssemblyResolver service?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/302#issuecomment-18499593
.

…blyResolver reads it.

khellang · 2013-05-27T16:08:37Z

What do you guys think? I added ScriptManifest which is written to the script root folder on installation, this can be extended with other things later. It is updated/created on package installation and read on script/REPL startup through IAssemblyResolver...

Now the only thing left is lots of tests 😄

khellang · 2013-05-27T16:11:33Z

This PR became a bit bigger than first intended... 😟

glennblock · 2013-05-27T16:25:38Z

Break it up ;-)

On Mon, May 27, 2013 at 9:11 AM, Kristian Hellang
[email protected]:

This PR became a bit bigger than first intended... [image: 😟]

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/302#issuecomment-18505482
.

khellang · 2013-05-27T16:48:05Z

I'm not sure how to split it up and leave the codebase in a good state. It isn't that big, functionality wise, but some renaming can make it seem big because it touches a lot of files... 28 files is a lot for this small codebase 😉

glennblock · 2013-05-27T17:22:27Z

I didn't even look at it, I just responded off the cuff ;p

On Mon, May 27, 2013 at 9:48 AM, Kristian Hellang
[email protected]:

I'm not sure how to split it up and leave the codebase in a good state. It
isn't that big, functionality wise, but some renaming can make it seem big
because it touches a lot of files... 28 files is a lot for this small
codebase [image: 😉]

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/302#issuecomment-18506826
.

…ded tests for IAssemblyResolver

glennblock · 2013-06-06T07:56:22Z

OK :-)

khellang · 2013-06-06T07:56:37Z

@glennblock That's probably because as of now, the exceptions are swallowed....

glennblock · 2013-06-06T07:56:40Z

But try out the webapi and nancy samples since both should break for you.

khellang · 2013-06-06T07:57:01Z

Will do 😄

glennblock · 2013-06-06T07:57:01Z

@khellang right i just merged that

khellang · 2013-06-06T08:00:13Z

There are some things though... How do I know which package folders I should pick up with MEF? For a package, there could be net40, sl4, etc. I could use the PackageAssemblyResolver, but don't have an instance of that in the CompositionRoot. I could new it up and register the instance with Autofac, but then I would also need to new up its dependencies... See the problem?

khellang · 2013-06-06T22:24:39Z

src/ScriptCs/CompositionRoot.cs

The AssemblyResolver is used to get assemblies to include in the AggregateCatalog. It's later reused when running a script/REPL for passing assemblies to reference in the ScriptExecutor. Should we cache these assembly names?

Yes I would

-----Original Message-----
From: "Kristian Hellang" [email protected]
Sent: ‎6/‎6/‎2013 3:24 PM
To: "scriptcs/scriptcs" [email protected]
Cc: "Glenn Block" [email protected]
Subject: Re: [scriptcs] Removed RestoreCommand and made assemblies load frompackages/root folder (#302)

In src/ScriptCs/CompositionRoot.cs:

{

var scriptPath = Path.Combine(Environment.CurrentDirectory, "bin");

if (Directory.Exists(scriptPath))

{

var catalog = new DirectoryCatalog(scriptPath);

builder.RegisterComposablePartCatalog(catalog);

}

var currentDirectory = Environment.CurrentDirectory;

var assemblies = assemblyResolver.GetAssemblyPaths(currentDirectory);
The AssemblyResolver is used to get assemblies to include in the AggregateCatalog. It's later reused when running a script/REPL for passing assemblies to reference in the ScriptExecutor. Should we cache these assembly names?
—
Reply to this email directly or view it on GitHub.

I would make the resolver cache

-----Original Message-----
From: "Kristian Hellang" [email protected]
Sent: ‎6/‎6/‎2013 3:24 PM
To: "scriptcs/scriptcs" [email protected]
Cc: "Glenn Block" [email protected]
Subject: Re: [scriptcs] Removed RestoreCommand and made assemblies load frompackages/root folder (#302)

In src/ScriptCs/CompositionRoot.cs:

{

var scriptPath = Path.Combine(Environment.CurrentDirectory, "bin");

if (Directory.Exists(scriptPath))

{

var catalog = new DirectoryCatalog(scriptPath);

builder.RegisterComposablePartCatalog(catalog);

}

var currentDirectory = Environment.CurrentDirectory;

var assemblies = assemblyResolver.GetAssemblyPaths(currentDirectory);
The AssemblyResolver is used to get assemblies to include in the AggregateCatalog. It's later reused when running a script/REPL for passing assemblies to reference in the ScriptExecutor. Should we cache these assembly names?
—
Reply to this email directly or view it on GitHub.

khellang · 2013-06-06T22:56:45Z

Now the caching should be good. Went from 226ms to 113ms 😃

glennblock · 2013-06-06T23:14:57Z

Cool

-----Original Message-----
From: "Kristian Hellang" [email protected]
Sent: ‎6/‎6/‎2013 3:56 PM
To: "scriptcs/scriptcs" [email protected]
Cc: "Glenn Block" [email protected]
Subject: Re: [scriptcs] Removed RestoreCommand and made assemblies load frompackages/root folder (#302)

Now the caching should be good. Went from 226ms to 113ms
—
Reply to this email directly or view it on GitHub.

glennblock · 2013-06-11T05:50:32Z

@khellang just tested it out, it is making big progress!

Web API hosting sample worked without a hitch. I ran the sample with just packages.config, it installed them and worked perfect!

However, the RavenDB sample did not. I did the same and ran it and got this output.

C:\code\scriptcs\scriptcs-samples\ravendb [master +1 ~2 -1 !]> scriptcs start.csx
INFO : Installing packages...
INFO : Installed: RavenDB.Embedded 2.0.2261.0
INFO : Installation completed successfully.
INFO : Starting to create execution components
INFO : Starting execution
INFO : Finished execution
ERROR: Roslyn.Compilers.CompilationErrorException: (2,7): error CS0246: The type or namespace name 'Raven' could not be
found (are you missing a using directive or an assembly reference?)
   at Roslyn.Scripting.CommonScriptEngine.CompilationError(DiagnosticBag localDiagnostics, DiagnosticBag diagnostics)
   at Roslyn.Scripting.CommonScriptEngine.Compile(String code, String path, DiagnosticBag diagnostics, Session session,
Type delegateType, Type returnType, CancellationToken cancellationToken, Boolean isInteractive, Boolean isExecute, Commo
nCompilation& compilation, Delegate& factory)
   at Roslyn.Scripting.CommonScriptEngine.CompileSubmission[T](String code, Session session, String path, Boolean isInte
ractive)
   at Roslyn.Scripting.Session.CompileSubmission[T](String code, String path, Boolean isInteractive)
   at ScriptCs.Engine.Roslyn.RoslynScriptEngine.Execute(String code, Session session) in c:\code\scriptcs\scriptcs\src\S
criptCs.Engine.Roslyn\RoslynScriptEngine.cs:line 93
C:\code\scriptcs\scriptcs-samples\ravendb [master +1 ~2 -1 !]>

So for some reason even though installed the packages it did not work. Now, after it errored out, I checked the packages folder and all the packages were there. I then ran it again and it worked.

I haven't tried to debug it yet, but it sounds like it either failed to load or tried to load too early.

glennblock · 2013-06-11T05:51:58Z

@khellang I just removed my packages folder and tested again, same result. So it's easy to repro using the RavenDB sample.

Also if I do "scriptcs -install" first and then run it works fine as well.

khellang · 2013-06-11T06:24:06Z

@glennblock that is actually not a problem with this PR, it's been there for a while. See #297. It only happens on automatic installation because the binaries are resolved at startup, and because there are no binaries at startup (hence the automatic installation), it fails when trying to run. Don't know if that's understandable 😄 We need to think about how we can do this in a two phase fashion, i.e. run automatic installation before bootstrapping the CLI app...

glennblock · 2013-06-11T08:25:23Z

Yes, I think what we should do here is basically tear down the container if
we go down the install path, and then just have it start over. This will be
the minimum amt of work to achieve what we want. The small hit of tearing
down the container and recreating it, is small and it only effects startup
if installation occurs.

On Mon, Jun 10, 2013 at 11:24 PM, Kristian Hellang <[email protected]

wrote:

@glennblock https://github.com/glennblock that is actually not a
problem with this PR, it's been there for a while. See #297 #297.
It only happens on automatic installation because the binaries are resolved
at startup, and because there are no binaries at startup (hence the
automatic installation), it fails when trying to run. Don't know if that's
understandable [image: 😄] We need to think about how we can do this
in a two phase fashion, i.e. run automatic installation before
bootstrapping the CLI app...

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/302#issuecomment-19244963
.

glennblock · 2013-06-12T04:20:51Z

This looks good enough to merge now so I am going to push the button. We can still go back and refactor the setup code that I commented on above, but it's not critical.

It'll be great to have this feature!

glennblock · 2013-06-12T04:21:03Z

Merged!

Removed RestoreCommand and made assemblies load from packages/root folder

khellang added 7 commits May 26, 2013 14:13

Removed RestoreCommand

f9107cf

Fixed failing test

f855757

Removed bin folder logic in CleanCommand

f1dccbf

Added SearchOption parameter to FileSystem.EnumerateFiles

1b3ad3a

Changed registering of MEF composable part catalog

ecb33fb

Added common base class for ExecuteScript- and ExecuteReplCommand

624c6bc

Added variable

83f21b5

glennblock reviewed May 27, 2013
View reviewed changes

ghost assigned glennblock May 27, 2013

Merged upstream changes

0b655d1

Removed ScriptCommand base class and renamed IAssemblyName to IAssemb…

c8df9c9

…lyResolver

khellang added 2 commits May 27, 2013 18:02

Added ScriptManifest, InstallCommand writes the manifest, while Assem…

7540fc3

…blyResolver reads it.

Fixed bug where manifest wasn't deserialized

e583fe0

khellang added 2 commits May 27, 2013 23:02

Added more logging

3f72a1f

Refactored out check for managed assemblies into IAssemblyUtility, ad…

a07ea4d

…ded tests for IAssemblyResolver

khellang added 5 commits June 6, 2013 21:55

Fixed bug where all assemblies inside working dir was picked up by MEF

5416721

Removed manifest stuff after perf. testing NuGet API alternative

c8e5712

Merge branch 'dev' into 1

2305c93

Fixed failing test

f007f42

Renamed method to a more appropriate name :)

9b850d4

khellang reviewed Jun 6, 2013
View reviewed changes

khellang added 2 commits June 7, 2013 00:49

Added caching of assembly paths in AssemblyResolver

d46626a

Removed static cache because of failing tests

c3f2403

Trigger CI build

80388d0

khellang added 2 commits June 9, 2013 12:58

Merged upstream changes

ba603fe

Fixed failing build after merge

12a0490

glennblock added a commit that referenced this pull request Jun 12, 2013

Merge pull request #302 from khellang/1

5517168

Removed RestoreCommand and made assemblies load from packages/root folder

glennblock merged commit 5517168 into scriptcs:dev Jun 12, 2013

khellang mentioned this pull request Jun 12, 2013

System.IO.FileNotFoundException using scriptcs -install on Chocolatey 0.5.0 build #321

Closed

ztone pushed a commit to ztone/scriptcs that referenced this pull request May 4, 2014

Merge pull request scriptcs#302 from khellang/1

9ffe756

Removed RestoreCommand and made assemblies load from packages/root folder

Conversation

khellang commented May 26, 2013

Uh oh!

glennblock commented May 27, 2013

Uh oh!

glennblock May 27, 2013

Choose a reason for hiding this comment

Uh oh!

khellang commented May 27, 2013

Uh oh!

glennblock commented May 27, 2013

Uh oh!

khellang commented May 27, 2013

Uh oh!

glennblock commented May 27, 2013

Uh oh!

khellang commented May 27, 2013

Uh oh!

jrusbatch commented May 27, 2013

Uh oh!

khellang commented May 27, 2013

Uh oh!

khellang commented May 27, 2013

Uh oh!

glennblock commented May 27, 2013

Uh oh!

khellang commented May 27, 2013

Uh oh!

khellang commented May 27, 2013

Uh oh!

glennblock commented May 27, 2013

Uh oh!

khellang commented May 27, 2013

Uh oh!

glennblock commented May 27, 2013

Uh oh!

glennblock commented Jun 6, 2013

Uh oh!

khellang commented Jun 6, 2013

Uh oh!

glennblock commented Jun 6, 2013

Uh oh!

khellang commented Jun 6, 2013

Uh oh!

glennblock commented Jun 6, 2013

Uh oh!

khellang commented Jun 6, 2013

Uh oh!

khellang Jun 6, 2013

Choose a reason for hiding this comment

Uh oh!

glennblock Jun 6, 2013

Choose a reason for hiding this comment

Uh oh!

glennblock Jun 6, 2013

Choose a reason for hiding this comment

Uh oh!

khellang commented Jun 6, 2013

Uh oh!

glennblock commented Jun 6, 2013

Uh oh!

glennblock commented Jun 11, 2013

Uh oh!

glennblock commented Jun 11, 2013

Uh oh!

khellang commented Jun 11, 2013

Uh oh!

glennblock commented Jun 11, 2013

Uh oh!

glennblock commented Jun 12, 2013

Uh oh!

glennblock commented Jun 12, 2013

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!